All tests using .command() fail with command X not found errors

[tylerbutler]

Describe the bug

After updating an oclif project from @oclif/test 2.3.33 to 3.2.11, all tests that use .command() fail with errors like this:

Error: command generate:buildVersion not found
      at Config.runCommand (/home/tylerbu/code/FluidFramework/build-tools/node_modules/.pnpm/@oclif+core@3.26.4/node_modules/@oclif/core/lib/config/config.js:403:19)
      at async Object.run (/home/tylerbu/code/FluidFramework/build-tools/node_modules/.pnpm/@oclif+test@3.2.11/node_modules/@oclif/test/lib/command.js:23:28)
      at async Context.run (/home/tylerbu/code/FluidFramework/build-tools/node_modules/.pnpm/fancy-test@3.0.14/node_modules/fancy-test/lib/base.js:68:25)

To Reproduce

Unfortunately I don't have a super-small repro. This PR where I am trying to do the upgrade demonstrates the problem, but I recognize that it may not be easy to use that PR to reproduce the issue on your side.

Expected behavior

No problems finding commands.

Additional context

On a hunch I tried changing the command discovery strategy to explicit following the instructions in the oclif docs. I thought maybe that would point towards a root cause, but it had no effect. I have also tried running tests with and without a manifest present and that seems to make no difference.

[mdonnalley]

@tylerbutler You probably just need to ensure that your code is compiled before running the tests

[mdonnalley]

@tylerbutler Never mind, I just built your branch and repo'd the problem. I'll take a look

[mdonnalley]

@tylerbutler Okay, so this is an issue with workspaces (which have been a consistent pain point for this repo).

The problem is here where we attempt to figure out the root path of the CLI. A few patches have been made to fix that logic for workspace-based projects. Obviously, it's still not working for yours though.

I think my preferred solution is to simply set an environment variable that would override that root discovery logic. So you'd have to run tests like this:

OCLIF_TEST_ROOT=build-tools/packages/build-cli npm run test

Is that a tenable solution for you?

Also, worth noting that I'm soon going to work on a major revision of this project that will no longer use fancy-test (i.e. you'll have to rewrite all your tests). I point this out because it might be easiest for you to stick with your current version until that comes out. Just something to think about.

[tylerbutler]

@mdonnalley Thanks a lot for looking into this!

I think my preferred solution is to simply set an environment variable that would override that root discovery logic. So you'd have to run tests like this:

OCLIF_TEST_ROOT=build-tools/packages/build-cli npm run test

Is that a tenable solution for you?

Yes, that would work for us. It seems like a useful "escape hatch" as well for other cases you may come across too.

Would it need to be an absolute path? Relative to the root of the workspace? Root of the repo? In our case the workspace root and repo root are different since our repo houses multiple workspaces.

I assume that approach would work for both ESM and CJS commands? Incidentally, our current CLI is CJS, but I have been working to convert it to ESM. When using ESM, if I downgrade to oclif/test 2.33.3, the tests fail with a trace that seems to imply that version of oclif/test doesn't work with ESM:

TypeError: Cannot read properties of undefined (reading 'filename')
    at Object.<anonymous> (/home/tylerbu/code/Fluid-bt/build-tools/node_modules/.pnpm/@oclif+test@2.3.33_fw7rjau6lbmauwrvieqeq6x7va/node_modules/@oclif/test/lib/index.js:14:47)
    at Module._compile (node:internal/modules/cjs/loader:1256:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
    at require.extensions.<computed> (/home/tylerbu/code/Fluid-bt/build-tools/node_modules/.pnpm/ts-node@10.9.2_fw7rjau6lbmauwrvieqeq6x7va/node_modules/ts-node/src/index.ts:1608:43)
    at Object.require.extensions.<computed> [as .js] (/home/tylerbu/code/Fluid-bt/build-tools/node_modules/.pnpm/ts-node@10.9.2_6aghfyf5eabo7u6nxooxqsbtpq/node_modules/ts-node/src/index.ts:1608:43)
    at Module.load (node:internal/modules/cjs/loader:1119:32)
    at Function.Module._load (node:internal/modules/cjs/loader:960:12)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:169:29)
    at ModuleJob.run (node:internal/modules/esm/module_job:194:25)

But the 3.x version fails with the same "command not found" error as CJS with 3.x, so I'm hopeful that your proposed env variable will unblock our ESM upgrade as well :)

Also, worth noting that I'm soon going to work on a major revision of this project that will no longer use fancy-test (i.e. you'll have to rewrite all your tests). I point this out because it might be easiest for you to stick with your current version until that comes out. Just something to think about.

Thanks for pointing this out. Is there anything I can read to learn more about your plans? No worries if not - I'll just watch the release notes.

[git2gus]

This issue has been linked to a new work item: W-15643461

[mdonnalley]

Would it need to be an absolute path? Relative to the root of the workspace? Root of the repo? In our case the workspace root and repo root are different since our repo houses multiple workspaces.

I think absolute path would be the only feasible way to do it. But I think I might have stumbled upon a solution that won't require the env var. Do you mind trying out @oclif/test@3.2.12-dev.0?

When using ESM, if I downgrade to oclif/test 2.33.3, the tests fail with a trace that seems to imply that version of oclif/test doesn't work with ESM

A little history: 2.3.33 used module.parent.filename as the root, which is undefined when using ESM. So we "fixed" that in 2.4.0 (#301), but that fix is now what causes all our issues with workspaces (just can't win 😞)

But hopefully #543 can finally allow folks to do ESM + workspaces

Thanks for pointing this out. Is there anything I can read to learn more about your plans? No worries if not - I'll just watch the release notes.

I haven't posted anything public yet because I'd like to get a little further into development first to understand more of what it will look like. However, you can see what I've developed so far.

This is the first iteration of the new @oclif/test (currently lives inside of @oclif/core but will eventually be moved to this repo)

And here are a couple of examples of how it's used:

• https://github.com/oclif/core/blob/prerelease/beta/test/command/main.test.ts#L12
• https://github.com/oclif/core/blob/prerelease/beta/test/command/command.test.ts#L23

For context, we want to move away from fancy tests for a few reasons:

• it assumes people will use mocha as their test runner
• there's a learning curve to writing fancy tests. Most people know how like to write their tests and don't want to bother with learning a new test framework
• fancy-test offers a lot of useful utilities (like stubbing process.env) but it doesn't do everything that people need so people end up writing tests that are half fancy-tests and half "unfancy" tests. I'd rather enable people to write tests however they want.
• we don't have bandwidth to support it as fully as we would like

So the goal with the next version is to expose a few helpful utilities (like captureOutput which will capture everything that goes to stderr or stdout) which people can use in whichever test runner they want to use and which allows them to write tests however they want.

[tylerbutler]

Do you mind trying out @oclif/test@3.2.12-dev.0?

Tested this out and it seems to work for our repo!

[tylerbutler]

Do you mind trying out @oclif/test@3.2.12-dev.0?

Tested this out and it seems to work for our repo!

Well, it works for our CJS package. In the ESM package it works with the OCLIF_TEST_ROOT variable but not without. I'm going to see how we can get that set to the absolute path in CI and locally. It may be a little tricky but I think we can do it.

[mdonnalley]

Well, it works for our CJS package. In the ESM package it works with the OCLIF_TEST_ROOT variable but not without.

@tylerbutler which package is that? I've got your branch locally so I can help debug it

[tylerbutler]

Well, it works for our CJS package. In the ESM package it works with the OCLIF_TEST_ROOT variable but not without.

@tylerbutler which package is that? I've got your branch locally so I can help debug it

@mdonnalley I created a new PR branch with a cleaner repro: microsoft/FluidFramework#20957

I used the oclif generator to create a new ESM project, bumped the test version to your prerelease, and added a second test script that uses the env variable. Details are in the PR description but if you run into problems let me know.

[mdonnalley]

@tylerbutler Thanks for the repro 🏆

I have a solution for you that doesn't require any changes to @oclif/test:

import {expect, test as oclifTest} from '@oclif/test'

const test = oclifTest.loadConfig({root: import.meta.url})

describe('hello', () => {
  test
    .stdout()
    .command(['hello', 'friend', '--from=oclif'])
    .it('runs hello cmd', (ctx) => {
      expect(ctx.stdout).to.contain('hello friend from oclif!')
    })
})

[mdonnalley]

FYI, I merged #543 and published 3.2.12 since it solves the workspaces issue for CJS projects. For ESM + workspaces, you can use the OCLIF_TEST_ROOT env var or use the workaround I posted above.

Happy to keep collaborating though if you think there's more that needs to be solved here.

[tylerbutler]

import {expect, test as oclifTest} from '@oclif/test'

const test = oclifTest.loadConfig({root: import.meta.url})

This is awesome. I think we're unblocked at this point. I appreciate the help. I'll continue my work and if I run into something odd I'll let you know.