chore: add tests for backend devcontainers plugin by buenos-nachos · Pull Request #74 · coder/backstage-plugins

buenos-nachos · 2024-03-20T19:08:17Z

Closes #62

Changes made

Added test file for DevcontainersProcessor
Fixed a bug where we weren't actually using the custom user-defined devcontainers tag name

Notes

This was the first time I've ever written a backend test, so please rip this PR apart

buenos-nachos · 2024-03-20T19:12:29Z

+  const {
+    searchThrowCallback,
+    readUrlThrowCallback,
+    tagName = DEFAULT_TAG_NAME,
+  } = options ?? {};


Wasn't sure if there was a better way to simulate errors for the reader callbacks

buenos-nachos · 2024-03-20T19:39:17Z

+    try {
+      const jsonUrl = await this.findDevcontainerJson(rootUrl, entityLogger);
+      entityLogger.info('Found devcontainer config', { url: jsonUrl });
+      return this.addTag(entity, this.options.tagName, entityLogger);


Main change here was making sure we weren't always passing in DEFAULT_TAG_NAME to this method call

Good catch!!

code-asher · 2024-03-21T19:40:33Z

+   * Tried to get this working as more of an integration test that used MSW, but
+   * couldn't figure out how to bring in the right dependencies in time. So this
+   * is more of a unit test for now (which might be all we really need?)


Makes sense to me, I like this as a unit test.

code-asher · 2024-03-21T19:43:21Z

+    it('Should use Coder prefix in the output', () => {
+      const { processor } = setupProcessor();
+      const name = processor.getProcessorName();
+      expect(name).toMatch(new RegExp(`^${PROCESSOR_NAME_PREFIX}`));


Not a blocker or anything, but curious why we do not make this the full name and do a direct equals? Like this, although I guess we could skip exporting the prefix until we have multiple processors:

export const PROCESSOR_NAME_PREFIX = 'backstage-plugin-devcontainers-backend'; export const PROCESSOR_NAME = `${PROCESSOR_NAME_PREFIX}/devcontainers-processor`;

Additional note that currently we have backstage-plugin-devcontainers-backend//devcontainers-processor (double slash), definitely not a bug or anything but might not be what was intended.

So, it's a preemptive thing (and maybe I should remove it because we don't need it yet), but it is possible to have multiple instances of the same processor, and have the processor name be partially dynamic

I guess I was trying to add a test case that would be more future-proofed, but that extra slash absolutely wasn't intentional. Will remove

code-asher · 2024-03-21T19:46:44Z

+      await Promise.all(
+        otherEntityKinds.map(async kind => {
+          const inputEntity = { ...baseEntity, kind };
+          const inputSnapshot = structuredClone(inputEntity);


Woah how did I not know about structuredClone?? Neat.

Yeah, it doesn't work for all value types, but it's pretty handy. (It's mainly useful for JSON-serializable values. If you have an object that doesn't make every property enumerable, like any Error object, those properties will get skipped over during the copying)

Good to know, thank you!

code-asher · 2024-03-21T19:53:04Z

+      expect(inputEntity.metadata.tags).toEqual(inputSnapshot.metadata.tags);
+      expect(inputEntity).toEqual(inputSnapshot);


Do these test anything? We know these are equal since we created it with structuredClone just above, right? Ohhhh wait is it to test that the processor did not modify the input object?

Separate note, do we need to test metadata.tags separately? toEqual is a recursive check I think.

Yeah, these are checks to make sure nothing was modified. I originally had comments here, but thought they might be overkill. I'll add them back in, just in case

And yeah, you're right – the check is fully recursive. Will remove the extra case

I think normally the comments would be overkill but I was going into the review with the expectation that we just need to check that the tags were added, so I was not thinking about also testing that the object was unmodified. Another way of doing it I think would be two separate tests like:

it("adds tag") it("does not modify object")

or combined or something like that.

code-asher · 2024-03-21T19:55:38Z

+      expect(inputEntity.metadata.tags).toEqual(inputSnapshot.metadata.tags);
+      expect(inputEntity).toEqual(inputSnapshot);
+
+      expect(outputEntity.metadata.tags).toContain(DEFAULT_TAG_NAME);


Would it make sense to check that none of the other fields have been modified? That might be overstepping the goal of the test, but since we are checking that the input was not modified it might make sense to check that no other fields were modified in the output.

Yeah, it's a tricky one to decide where the boundaries are. It's not too hard to set up the assertions for the other fields, though, so I can add these in really quick

code-asher · 2024-03-21T19:56:49Z

+      expect(inputEntity.metadata.tags).toEqual(inputSnapshot.metadata.tags);
+      expect(inputEntity).toEqual(inputSnapshot);
+
+      expect(outputEntity.metadata.tags).toContain(customTag);


Same comments as above.

code-asher · 2024-03-21T19:57:43Z

+      expect(outputEntity.metadata.tags).toContain(customTag);
+    });
+
+    it('Emits an error entity when reading from the URL throws anything other than a NotFoundError', async () => {


I really like these error case tests!

code-asher · 2024-03-21T20:07:05Z

+  ) => never;
+}>;
+
+function setupProcessor(options?: SetupOptions) {


Could we make the mock take in files so we can test the various locations? Right now we always return a mock file or always return an error, but if we could do something like:

// these get a tag setupProcessor({ files: { ".devcontainer.json": "blah", }, }) setupProcessor({ files: { ".devcontainer/nested/container.json": "blah", }, }) // this does not get a tag setupProcessor({ files: { ".devcontainer/nested/too/much/container.json": "blah", }, }) // and so on

Then we could test all the permutations (anything not specified returns NotFoundError). Although, the second and third examples will require parsing a glob, but I think we can just import the glob library and compare with each file, which will be perfect so we can make sure we are using the glob correctly as well.

Oh, yeah. Good call. I'm going to try fiddling with this, but when you say "glob library", do you mean the one on NPM?

Also, just making sure: would files be an object, or an array?

I think you could do it either way, I went with an object because I think it is slightly simpler on the caller side, but it could be annoying if we need more properties like the file mode or whatever, so array could be more future-proof.

Also idk but this might be helpful: https://github.com/tschaub/mock-fs

Which we can maybe combine with https://github.com/isaacs/node-glob

I think it would let us do something like:

mock(files)

and then for search we additionally have:

await glob(theGlob)

or something like that.

That makes sense. I can read up on globs and how to handle them – my main concern is because this PR has a fix for the main processor functionality, too, I don't know how long that would delay me wrapping things up

Would you be okay with approving this PR, as long as I make a new issue for tetsting the glob functionality?

Yup, no worries. Hopefully it is not too rough, the glob library should handle it all if you mock the filesystem, but reasonable to do it in another PR I think.

Cool. Thanks!

buenos-nachos added 26 commits March 19, 2024 16:27

refactor: split processor off into separate file

714a3b5

chore: add initial version of processor test file

89c1d8f

chore: add stubs for all test cases

db33685

refactor: update getProcessorName to use prefix constant

524368f

chore: finish test case for other entity kinds

734ecf3

refactor: update test case to run test situations in 'parallel'

46c58ef

chore: finish test case for location specs

674741c

wip: add stub for append test case

f25bce0

fix: update input setup for test case

56df26d

wip: commit current test progress

f136690

refactor: clean up processor code

d53cbfd

wip: commit progress for tests

a4bb4b4

refactor: move static locations outside class method

30bab33

refactor: move static locations outside class method

ea0e52c

fix: remove console.log call

bb69cfe

refactor: rename input type for class

c077965

chore: add msw to dev dependencies

ba34410

fix: rename misleading variable

9a32962

refactor: replace custom logger with void logger

d879ee8

fix: make sure custom tag name is passed to processor

cb392cf

wip: finish stubs for all test cases

a34524f

fix: remove needless assertion safety net

632a356

fix: remove typo in test filename

c6fefa4

fix: remove accidental logs

4f33552

wip: commit current progress on tests

f93e6c3

chore: finish initial version of devcontainers test

d156dcf

buenos-nachos requested a review from code-asher March 20, 2024 19:08

buenos-nachos self-assigned this Mar 20, 2024

buenos-nachos commented Mar 20, 2024

View reviewed changes

Comment thread plugins/backstage-plugin-devcontainers-backend/src/index.ts

buenos-nachos commented Mar 20, 2024

View reviewed changes

code-asher reviewed Mar 21, 2024

View reviewed changes

buenos-nachos added 5 commits March 22, 2024 14:20

fix: remove typo slash

202f996

refactor: clean up tests

0a2c57a

fix: add extra checks to ensure no mutations happened

a95c736

Merge branch 'main' into mes/devcontainers-be-tests

bc8dbd6

refactor: add support for custom mock files

b51a303

code-asher approved these changes Mar 22, 2024

View reviewed changes

buenos-nachos mentioned this pull request Mar 22, 2024

Backend Devcontainers plugin: Add tests for glob functionality #77

Open

buenos-nachos merged commit 07da627 into main Mar 22, 2024

buenos-nachos deleted the mes/devcontainers-be-tests branch March 22, 2024 20:04

		expect(inputEntity.metadata.tags).toEqual(inputSnapshot.metadata.tags);
		expect(inputEntity).toEqual(inputSnapshot);

Conversation

buenos-nachos commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes made

Notes

Uh oh!

Uh oh!

buenos-nachos Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buenos-nachos Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

buenos-nachos commented Mar 20, 2024 •

edited

Loading

buenos-nachos Mar 20, 2024 •

edited

Loading

code-asher Mar 21, 2024 •

edited

Loading

buenos-nachos Mar 22, 2024 •

edited

Loading

code-asher Mar 22, 2024 •

edited

Loading

code-asher Mar 21, 2024 •

edited

Loading

code-asher Mar 22, 2024 •

edited

Loading