Restructure test files and Eslint improvements

[EhabY]

Refactors test layout, tightens linting, and adds path aliases.

Changes

• Move tests out of /src into a top-level /test directory:
  • /test/unit — unit tests with mocks
  • /test/integration — end-to-end tests
  • /test/mocks — shared mocks and test helpers
  • /test/fixtures — moved from /fixtures
  • /test/utils — varies test utils
• Configure ESLint and TypeScript “fix on save” in VS Code.
• Address several ESLint rules (imports, typing).
• Update ESLint-related dependencies.
• Introduce path aliases (for tests):
  • @ → /src
• Add ServiceContainer that initializes and tracks all services in src/core
• Make it impossible to import test files in src
• Unify fixture path resolution
• Improve .vscodeignore, now we only ship what we use exactly

[code-asher]

Going to review soon, but wanted to comment first, are we sure we want to move the test files into a separate directory? I actually quite like having them adjacent to the files they test, and we do the same in coder/coder.

I assume it would still be possible to write the import rule even if they were co-located.

[code-asher]

Broadly looks good to me! I do want to align on moving the test files before we merge.

[EhabY]

I lean toward separating test and src for a few reasons:

• Keeps test utilities from accidentally being imported into production code (like a testUtils.ts file ending up in the bundled extension).
• Mock types (especially for the vscode module) won't pollute IntelliSense when working in source files.
• We can have separate tsconfig and ESLint rules for tests. Tests often need looser type checking for mocks that we don't want in production.
• Since both integration and unit tests use *.test.ts, having test/unit/ and test/integration/ folders makes it obvious which tests need the VS Code Extension Host and which don't (and makes it possible for vitest and vscode-test to differentiate)
• GitLens, vscode-cpptools, and Microsoft's samples all do this. While co-location works great for coder/coder, VS Code extensions seem to have standardized on separation.

IMO all of the reasons above make it worthwhile to separate the source code from the test code.

[code-asher]

Keeps test utilities from accidentally being imported into production code (like a testUtils.ts file ending up in the bundled extension).

I think a src/testutils directory is a great idea, it is really just the tests themselves that would be co-located.

Mock types (especially for the vscode module) won't pollute IntelliSense when working in source files.

Could you expand more on when/where this would happen? I see the vscode alias in vitest.config.ts but am not sure how/why that would affect the editor when editing source files.

We can have separate tsconfig and ESLint rules for tests. Tests often need looser type checking for mocks that we don't want in production.

This is a separate concern from the directory structure, no? We can have any kind of rule setup we want for any file or set of files.

Also, I wonder if we really do need looser type checking, maybe this is a code smell that really means we need narrower production types so our mocks can implement them fully.

Since both integration and unit tests use *.test.ts, having test/unit/ and test/integration/ folders makes it obvious which tests need the VS Code Extension Host and which don't (and makes it possible for vitest and vscode-test to differentiate)

A .unit.test.ts and .integration.test.ts suffix would make it obvious as well, I think, if we need to differentiate at the file level.

GitLens, vscode-cpptools, and Microsoft's samples all do this. While co-location works great for coder/coder, VS Code extensions seem to have standardized on separation.

Convention is a strong argument, but navigating between test/source is more pleasant when they are co-located (or is this just me 😢), so if we can accomplish this and still make the tooling work, I think it would be worthwhile. I also like how it helps tell at a glance what files are missing tests, although that is arguably a job better for coverage tools.

[EhabY]

I think a src/testutils directory is a great idea, it is really just the tests themselves that would be co-located.

It's two things here though, we need to remember to add new test folders to .vscodeignore so we do not package them in the extension. But also, we can accidentally import them because we want to use a function with a similar name.

Could you expand more on when/where this would happen? I see the vscode alias in vitest.config.ts but am not sure how/why that would affect the editor when editing source files.

Oh I encountered this while I was trying to use something in vscode, IntelliSense detected that there is a "closer" vscode definition and offered to import workspace from my vscode.runtime.ts mock. This can applied to all other test utils. (This is more about the TS Config and less about vitest which is only used for tests anyway)

This is a separate concern from the directory structure, no? We can have any kind of rule setup we want for any file or set of files.

We can but similar to point (1), it'll be very hard to maintain and track since the files are intertwined. Here's an example for a rule that we might allow for tests but not for production:

"@typescript-eslint/consistent-type-imports": 
...
	"disallowTypeAnnotations": false, // Used in tests
...

This is because in tests to override modules we have to use type annotations:

vi.mock("fs", async () => {
	const memfs: { fs: typeof fs } = await vi.importActual("memfs");
	return {
		...memfs.fs,
		default: memfs.fs,
	};
});

A .unit.test.ts and .integration.test.ts suffix would make it obvious as well, I think, if we need to differentiate at the file level.

Yes, you are correct here, we can use different suffixes for different kinds of tests!

but navigating between test/source is more pleasant when they are co-located

Maybe we are used to different styles of programming 😓, but I usually just trigger the "Open File" command and type the name of the file (e.g. cliManager/cliManager.test) so it's intuitive, so I don't see that big of a value for co-location. If anything, the intertwining will make the setup harder and more issues might popup later.

For coverage, we already get one if you run the test using yarn test:(ci) --coverage. I even tried to add coverage report as a comment on the PR but backed off after a few attempts because wanted to do some research there. I can create another issue to add coverage report on PRs so contributors are immediately aware of this, we can even fail the pipeline on insufficient coverage but that might be overkill.

[code-asher]

It's two things here though, we need to remember to add new test folders to .vscodeignore so we do not package them in the extension.

There would only be the one test directory testutils right? But also this is true with the test directory setup as well. If you add another test directory, you would have to ignore it as well. I am not sure why one would do that in either case though; the one directory seems enough to me in both cases.

But also, we can accidentally import them because we want to use a function with a similar name.

If we have test utilities in a separate directory like testutils how would you accidentally import from there? You would have to type from ../testutils.

I could see it being an issue with auto-importing via the language server or something, but a test directory would have the same problem. If we need to worry about this, ideally we would have a linting rule preventing such things. I wonder if we can configure auto-import rules?

Oh I encountered this while I was trying to use something in vscode, IntelliSense detected that there is a "closer" vscode definition and offered to import workspace from my vscode.runtime.ts mock. This can applied to all other test utils. (This is more about the TS Config and less about vitest which is only used for tests anyway)

Ohhhhhh so not that the types resolve incorrectly for existing imports, but that it auto-imports the wrong thing. Oof. And putting the mocks in a test directory fixes this auto-import behavior? I think putting the mocks in the testutils directory would make sense, there is nothing for them to be co-located with anyway.

We can but similar to point (1), it'll be very hard to maintain and track since the files are intertwined. Here's an example for a rule that we might allow for tests but not for production:

I wonder if it would be better to disable rules in the test source code rather than globally by config. That way, a dev has to explicitly opt into losing type safety, if that makes sense.

Do you mean the rule will be hard to maintain? The rule is the same except we use "files: ["*.test.ts"] right?

I usually just trigger the "Open File" command and type the name of the file (e.g. cliManager/cliManager.test) so it's intuitive, so I don't see that big of a value for co-location.

Yup this is exactly what I would do, and I find it slower than using file navigation, which is fewer keypresses and less cognitive overhead because I just have to navigate up/down once without having to remember and type out the file name.

Also the number of times I type cliManager and for some reason the test one comes up first for whatever reason and that one opens when I instinctively hit enter yet I wanted the source code...so then I have to type it again, but then explicitly navigate to the second entry, drives me insane lol

If you have multiple matches, like multiple cli* files, then you have to think about which one and select the right one, or type the name more fully. Gets worse if you have files with the same name, like multiple util.test.ts or whatever.

If anything, the intertwining will make the setup harder and more issues might popup later.

How so? I have not noticed any issues with it on coder/coder.

Tests are also code, and they are highly coupled with the source they test. It makes sense to me that the file structure reflects the coupling.

I know this is a lot of text for just some file locations lol, sorry. I am learning I apparently feel stronger about this than I realized.

For coverage, we already get one if you run the test using yarn test:(ci) --coverage. I even tried to add coverage report as a comment on the PR but backed off after a few attempts because wanted to do some research there. I can create another issue to add coverage report on PRs so contributors are immediately aware of this, we can even fail the pipeline on insufficient coverage but that might be overkill.

Yeah we have tried to enforce coverage a few times I think on various repositories, but it seems to always go poorly and we remove it haha. So this makes sense to me. Being aware is nice, but blocking on it might be too much.

[EhabY]

But also this is true with the test directory setup as well. If you add another test directory, you would have to ignore it as well.

That is true yes, but here everything test related lives in test, there's always one place. But the previous structure had other folders like src/test (integration tests)/src/testUtils//fixtures. I can see this potentially growing with integration tests where we might need more files for test setup. All test files being around one another is less error-prone.

If we have test utilities in a separate directory like testutils how would you accidentally import from there? You would have to type from ../testutils

I meant auto-imports yes, not manual. Though by using separate tsconfigs then it's actually impossible to build if you do that manually (not that it's an issue obviously since why would you do that deliberately 😅).

And putting the mocks in a test directory fixes this auto-import behavior?

In this case yes, because the src cannot "see" anything in test (while test can see all of src).

Do you mean the rule will be hard to maintain? The rule is the same except we use "files: ["*.test.ts"] right?

I meant rules that could be in test util files, even though they have .ts extension. We can of course work around that by adding rules like src/testutils/*.ts.

How so? I have not noticed any issues with it on coder/coder.

It's mostly having a more difficult setup. We have to configure both tsconfig and eslint to so that test utils are treated in a special way even though they live in the same folders as src. I can see this accidentally happening, or creating a util that is solely for tests but can be used by production code.

Yeah we have tried to enforce coverage a few times I think on various repositories, but it seems to always go poorly and we remove it haha. So this makes sense to me. Being aware is nice, but blocking on it might be too much.

Fair enough haha, I am for just commenting or showing that percentage somewhere in the PR. Doesn't have to be blocking.

[code-asher]

That is true yes, but here everything test related lives in test, there's always one place. But the previous structure had other folders like src/test (integration tests)/src/testUtils//fixtures

Ah yeah good point, I think I am advocating a kind of hybrid, where we do move as much as we can into a single testutils directory, and only have .test.ts files outside of that dir that correlate to the file they test.

I meant auto-imports yes, not manual. Though by using separate tsconfigs then it's actually impossible to build if you do that manually (not that it's an issue obviously since why would you do that deliberately 😅).

Aaahhhh I see, we prevent the auto-import by having a completely separate tsconfig. I wonder if we can do that on a per-file basis? Maybe not. Although wait, does it still auto-import if we exclude .test.ts files? Although, ideally the .test.ts files are not exporting anything anyway, any test exports should all be in testutils. 🤔

It's mostly having a more difficult setup

Setup is one-time, reading and writing code is daily! Tools should work for humans, but it kinda feels like we are doing the reverse and adapting for the sake of the tools, if that makes sense.

We have to configure both tsconfig and eslint to so that test utils are treated in a special way even though they live in the same folders as src

I definitely like the idea of a separate testutils directory, no need to co-locate test utils.

[EhabY]

Ah yeah good point, I think I am advocating a kind of hybrid, where we do move as much as we can into a single testutils directory, and only have .test.ts files outside of that dir that correlate to the file they test.

Ah so so fixtures and mocks can be moved to testutils, and that is located inside src or outside? In any case, this is def better than the current way where things are a bit more scattered.

does it still auto-import if we exclude .test.ts files? Although, ideally the .test.ts files are not exporting anything anyway, any test exports should all be in testutils. 🤔

Test files should not export anything anyway, I was talking about code in mocks or test helpers. I think we can have something in the tsconfig for production that excludes that folder I suppose.

Setup is one-time, reading and writing code is daily!

Very fair point yes, if it can be done then yeah I am not against it (still prefer the separate approach haha).

[code-asher]

For anyone interested, we tried the co-location and unfortunately it turns out if we want to prevent cross-imports between tests and source, we have to keep them in separate directories. Seems like the tsconfig just does not give us the option to specify different sets of included files on a per-file-basis; it only works on a directory basis. No way to separate things like you can with Go's package directive, all it can do is traverse upward until it finds a tsconfig, and those are the rules that get applied to everything underneath, unconditionally.

We could have a linting rule after the fact to prevent merging cross-imports via CI, but that seems like a worse developer experience (I suppose the IDE could be configured to show linting errors while editing, but it would still be annoying to have the auto-import list populated by invalid entries).