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 incoder/coder.

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

I lean toward separatingtest andsrc for a few reasons:

• Keeps test utilities from accidentally being imported into production code (like atestUtils.ts file ending up in the bundled extension).
• Mock types (especially for thevscode module) won't pollute IntelliSense when working in source files.
• We can have separatetsconfig andESLint 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, havingtest/unit/ andtest/integration/ folders makes it obvious which tests need the VS Code Extension Host and which don't (and makes it possible forvitest andvscode-test to differentiate)
• GitLens, vscode-cpptools, and Microsoft's samples all do this. While co-location works great forcoder/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.

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

I think asrc/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 thevscode alias invitest.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.

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 invscode, IntelliSense detected that there is a "closer"vscode definition and offered to importworkspace from myvscode.runtime.ts mock. This can applied to all other test utils. (This is more about the TS Config and less aboutvitest 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()=>{constmemfs:{fs:typeoffs}=awaitvi.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 usingyarn 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.

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 directorytestutils right? But also this is true with thetest 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 liketestutils how would you accidentally import from there? You would have to typefrom ../testutils.

I could see it being an issue with auto-importing via the language server or something, but atest 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 atest directory fixes this auto-import behavior? I think putting the mocks in thetestutils 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 typecliManager 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 multiplecli* 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 multipleutil.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 oncoder/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.

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 intest, there's always one place. But the previous structure had other folders likesrc/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 separatetsconfigs 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 thesrc cannot "see" anything intest (whiletest can see all ofsrc).

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 likesrc/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 bothtsconfig andeslint to so that test utils are treated in a special way even though they live in the same folders assrc. I can see this accidentally happening, or creating autil 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.

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 singletestutils 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 intestutils. 🤔

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 separatetestutils directory, no need to co-locate test utils.

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 sofixtures andmocks can be moved totestutils, and that is located insidesrc 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 thetsconfig 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).