FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: skip some console tests on dumb terminal by AdamMajer · Pull Request #33165 · nodejs/node · GitHub

/ node Public

test: skip some console tests on dumb terminal - #33165

Closed
AdamMajer wants to merge 1 commit into
nodejs:masterfrom
AdamMajer:dumb-term-tests
ConversationCommits (1)ChecksFiles changed
Closed

test: skip some console tests on dumb terminal#33165
AdamMajer wants to merge 1 commit into
nodejs:masterfrom
AdamMajer:dumb-term-tests

Conversation

AdamMajer commented Apr 30, 2020

Copy link
Copy Markdown
Contributor

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

nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 30, 2020
richardlau reviewed Apr 30, 2020
View reviewed changes

richardlau left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott commented May 1, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented May 1, 2020

Copy link
Copy Markdown
Member

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 commented May 1, 2020

Copy link
Copy Markdown
Member

@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 approved these changes May 1, 2020
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM % comments

Comment thread test/common/index.js Outdated

AdamMajer commented May 2, 2020 via email

Copy link
Copy Markdown
Contributor Author

BridgeAR commented May 3, 2020

Copy link
Copy Markdown
Member

@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 commented May 4, 2020

Copy link
Copy Markdown
Member

@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.

test: skip some console tests on dumb terminal
abea223 
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'
        }
      ...

AdamMajer commented May 4, 2020

Copy link
Copy Markdown
Contributor Author

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 commented May 4, 2020

Copy link
Copy Markdown
Collaborator

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

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 4, 2020

nodejs-github-bot commented May 5, 2020

Copy link
Copy Markdown
Collaborator

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

addaleax commented May 7, 2020

Copy link
Copy Markdown
Member

Landed in df8db42

addaleax pushed a commit that referenced this pull request May 7, 2020
test: skip some console tests on dumb terminal
df8db42 
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'
        }
      ...

PR-URL: #33165
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax closed this May 7, 2020
codebytere pushed a commit that referenced this pull request May 11, 2020
test: skip some console tests on dumb terminal
302408e 
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'
        }
      ...

PR-URL: #33165
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere mentioned this pull request May 18, 2020
Merged
codebytere pushed a commit that referenced this pull request Jun 7, 2020
test: skip some console tests on dumb terminal
139eb6b 
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'
        }
      ...

PR-URL: #33165
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere mentioned this pull request Jun 9, 2020
Merged
AdamMajer deleted the dumb-term-tests branch September 16, 2020 10:46
AdamMajer mentioned this pull request Mar 16, 2021
Merged
aduh95 pushed a commit to AdamMajer/node that referenced this pull request May 5, 2024
test: skip some console tests on dumb terminal
394a0a6 
This adds two more tests to be skipped on systems with only a
dumb terminal. See nodejs#33165
for details.
nodejs-github-bot pushed a commit that referenced this pull request May 6, 2024
test: skip some console tests on dumb terminal
f76b28f 
This adds two more tests to be skipped on systems with only a
dumb terminal. See #33165
for details.

PR-URL: #37770
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
targos pushed a commit that referenced this pull request May 8, 2024
test: skip some console tests on dumb terminal
968fe6a 
This adds two more tests to be skipped on systems with only a
dumb terminal. See #33165
for details.

PR-URL: #37770
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
test: skip some console tests on dumb terminal
1dce5de 
This adds two more tests to be skipped on systems with only a
dumb terminal. See #33165
for details.

PR-URL: #37770
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
test: skip some console tests on dumb terminal
efcb5dc 
This adds two more tests to be skipped on systems with only a
dumb terminal. See nodejs#33165
for details.

PR-URL: nodejs#37770
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

richardlau richardlau left review comments
addaleax addaleax approved these changes
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL