test: skip some console tests on dumb terminal

[AdamMajer]

Add capabilities to common test module to detect and skip tests
on dumb terminals.

In some of our build environments, like s390x, the terminal
is a dumb terminal meaning it has very rudimentary capabilities.
These in turn prevent some of the tests from completing with errors
as below.

not ok 1777 parallel/test-readline-tab-complete
  ---
  duration_ms: 0.365
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:103
      throw new AssertionError(obj);
      ^

    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

    '\t' !== ''

        at /home/abuild/rpmbuild/BUILD/node-git.8698dd98bb/test/parallel/test-readline-tab-complete.js:63:14
        at Array.forEach (<anonymous>)
        at /home/abuild/rpmbuild/BUILD/node-git.8698dd98bb/test/parallel/test-readline-tab-complete.js:18:17
        at Array.forEach (<anonymous>)
        at Object.<anonymous> (/home/abuild/rpmbuild/BUILD/node-git.8698dd98bb/test/parallel/test-readline-tab-complete.js:17:3)
        at Module._compile (internal/modules/cjs/loader.js:1176:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:1196:10)
        at Module.load (internal/modules/cjs/loader.js:1040:32)
        at Function.Module._load (internal/modules/cjs/loader.js:929:14)
        at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: '\t',
      expected: '',
      operator: 'strictEqual'
    }
  ...

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

commit message follows guidelines except for copy-paste error message

[richardlau]

Please also (re-)export the new methods in test/common/index.mjs.

[Trott]

Skipping the tests on dumb terminals seems OK as something we can do to work around this issue right away, but I'd prefer we make sure functionality degrades gracefully for dumb terminals. So, for example, in the tab-completion test, outputting a tab character (which is what it appears to be doing) rather than doing tab-completion should result in a passing test on dumb terminals.

@BridgeAR

[richardlau]

FWIW we hit this in our CI last year: #28064
I started looking at making the tests degrade for dumb terminals (richardlau@b3b9e7e) but in the end we resolved by making TERM consistent across the machines in question so I didn't follow up.

[BridgeAR]

@Trott changing all existing tests to work with dumb terminals would be tricky. I think it is easier to just skip them and have dedicated tests for dumb terminals. That is how we currently handle it. Thus, I am +1 for this approach.

[BridgeAR]

LGTM % comments

[AdamMajer]

On 2020-05-01 2:20 p.m., Ruben Bridgewater wrote: ***@***.**** approved this pull request. LGTM % comments ---------------------------------------------------------------------------------------------------- In test/common/index.js <#33165 (comment)>: > @@ -112,6 +112,8 @@ const isOpenBSD = process.platform === 'openbsd'; const isLinux = process.platform === 'linux'; const isOSX = process.platform === 'darwin'; +const isDumbTerminal = process.env.TERM === 'dumb'; Should we really expose a property for a one liner? I would just inline it in the tests.

It's the same for the above one liners too, like isLinux. Furthermore, while TERM === 'dumb' seems to be OK for now, there could be some situation where console is in semi-dumb mode too when TERM is some other value. http://man7.org/linux/man-pages/man7/term.7.html If you use a dialup line, the type of device attached to it may vary. Older UNIX systems pre-set a very dumb terminal type like “dumb” or “dialup” on dialup lines. So I would prefer to keep this one liner. It shouldn't add too much overhead and the name should also not be confusing. - Adam

[BridgeAR]

@AdamMajer we thought about removing the other one liners as well. Ideally our tests use as few helper functions as possible. We could indeed support dialup while I doubt that we should really do this by now. Thus, it's unlikely to change. @Trott maybe you want to shim in about this?

[Trott]

@Trott maybe you want to shim in about this?

No opinion. Or rather I can see it both ways. I don't like to see more things added to the common monolith, but the case for this one is pretty good as such things go.

[AdamMajer]

If we are to remove anything, it could be "skipIfDumbTerminal" as that is where we will not see any changes in the future. But it's used in more than 10 places now. In comparison, we have skipIfWorker used in fewer places.

But like I've indicated above, isDumbTerminal is not so clear-cut scenario. It may expand to more than just TERM==='dumb'. It's used explicitly in only 2 places though.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31165/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31170/

[addaleax]

Landed in df8db42