Command parsing hangs under unit test

[john-u]

We have been using oclif v1 and unit test our CLI using jest. We follow the instruction in the oclif docs to use the static Command.run inside tests. When attempting to migrate the project from v1 to core, several of the tests had timeout failures which signified a hanging handler. I dug into the parsing differences between v1 and core and found that core has a new feature which falls back to reading from stdin if there are no arguments specified. This causes issues as our CLI has several commands which have only optional arguments. I am not aware of this being a known breaking change, as it isn't mentioned on the migration guide.

Bug

I believe a summary of the issue is that calling Command.run() without args under test causes a hang at this line in the parser (waiting for input that will never arrive).

Current (core) behavior

Here is an example of one of our tests that is hanging with core.

await expect(AppRegisterCommand.run()).resolves.not.toThrow() // note the omitted run args

It fails with the following.

: Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Timeout

The test just below it however passes on the same command since it includes args.

await expect(AppRegisterCommand.run([appId])).resolves.not.toThrow() // including any arg works around the hang

✕ calls selectFromList with correct config (5005 ms)
✓ calls correct endpoint for registration and logs success (18 ms)

Environment

"@oclif/core": "^1.0.11 "
darwin-x64 node-v12.18.1

[peternhale]

Hi @john-u, thank you for raising this issue. If possible, can you share the project you are working on so I might give it a go?

[john-u]

https://github.com/SmartThingsCommunity/smartthings-cli

[peternhale]

Thank you

[peternhale]

@john-u The master branch is using oclif v1 in cli package. Is there a branch I can fetch that has the changes to use core?

[john-u]

Yes sorry about that, I forgot to push my branch. Here is a PR SmartThingsCommunity/smartthings-cli#238

[peternhale]

@john-u I just ran the test you mentioned in the original post packages/cli/src/__tests__/commands/apps/register.test.ts. While it is failing, it is not hanging.

Error: expect(received).resolves.not.toThrow()

Received promise rejected instead of resolved
Rejected to value: [Error: Unexpected arguments: --reporters, /Applications/WebStorm.app/Contents/plugins/JavaScriptLanguage/helpers/jest-intellij/lib/jest-intellij-reporter.js, --verbose, --runTestsByPath, /Users/peter.hale/oclifProjects/smartthings-cli/packages/cli/src/__tests__/commands/apps/register.test.ts
See more help with --help]

Not what the state of this test might be.

Running on Mac 11.6.2
node --version
v16.13.1

[john-u]

@peternhale I noticed that as well while investigating this. If Command.run() has no args, oclif falls back to grabbing them from process. In this case, it doesn't hang but the args it got were meant for jest. I think the workaround for us here would be to ensure that an empty arg array is given to run Command.run([]) then oclif won't look at the process for args. I feel like this is related to the cause of the initially reported issue.

[peternhale]

@john-u I tried the suggestions of adding the [] to run. Now the test fails with a timeout.

: Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.
Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Error: 

I will discuss this with team mates, next week.

[peternhale]

@john-u I believe I have reproduced this using a simpler project. Command in question does define, so not sure if the timeout I am seeing is the same that you are seeing.

static args = [{name: 'person', description: 'Person to say hello to', required: true}]

Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Error:

      2 | 
      3 | describe('hello', async () => {
    > 4 |   it('runs hello cmd', async () => {
        |   ^
      5 |     await Hello.run([])
      6 |   })
      7 | })

[john-u]

@john-u I believe I have reproduced this using a simpler project. Command in question does define, so not sure if the timeout I am seeing is the same that you are seeing.

static args = [{name: 'person', description: 'Person to say hello to', required: true}]

Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Error:

      2 | 
      3 | describe('hello', async () => {
    > 4 |   it('runs hello cmd', async () => {
        |   ^
      5 |     await Hello.run([])
      6 |   })
      7 | })

If only one arg is specified as required, I would expect oclif to throw Missing 1 required arg instead of hanging, so I'm also not sure that Timeout is the same case. My mention about our commands having only optional args was just in reference to our need to unit test them without specifying any, as some logic may depend on the omission of args in those cases.

[tft7000]

Maybe I experienced the same problem but with @oclif/core itself:

I have a gitlab pipeline with semantic release for an oclif app that's based on @oclif/core.
Within npm publish the oclif manifest command (referenced in package.json prepack script) hangs. Unfortunatly I can only reproduce it within the pipeline but not manually. Neither in the agent container nor anywhere else.
However I could trace the problem to this line: https://github.com/oclif/core/blob/main/src/parser/parse.ts#L243 (that got initiated by the the oclif Manifest command's const {args} = await this.parse(Manifest) line here https://github.com/oclif/oclif/blob/main/src/commands/manifest.ts#L17):

// parser/parse.ts around line 243
if (!arg.ignoreStdin && !stdinRead) {
        // eslint-disable-next-line no-await-in-loop
        let stdin = await readStdin() // <--- here the script hangs in a automated pipeline
        if (stdin) {
          stdin = stdin.trim()
          args[i] = stdin
        }

        stdinRead = true
}

so my current workaround is to provide the default value for optional arguments.

[peternhale]

@tft7000 Thank you for raising this issue. We will have a look.

[MonochromeChameleon]

I've been looking into this today and it looks like the problem is in the parser inside that readStdin() call that @tft7000 identified. Where on line 26 of parse.ts it has:

if (stdin.isTTY) return result

the isTTY property isn't inherited by spawned child processes, and so is undefined rather than True. Changing it to

if (stdin.isTTY || stdin.isTTY === undefined) return result;

fixes the problem for me.

Happy to raise a PR over the weekend if that would be helpful

[mdonnalley]

@MonochromeChameleon @john-u 1.3.1 has the fix from #363 - can you let me know if you're still seeing this issue?

[tft7000]

@mdonnalley : from my side I can say that with the fix from #363 in v1.3.1 oclif manifest works now as expected within a pipeline. thanks @MonochromeChameleon for the work.

[MonochromeChameleon]

Everything is working for me with version 1.3.1 as well - thanks for getting it merged and released so quickly!

[john-u]

1.3.1 has resolved the hanging tests we were seeing as described above. Thank you!