Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: include component in tap output #6653

Merged
merged 1 commit into from
May 13, 2016
Merged

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented May 9, 2016

Print test name as (for example) "parallel/test-assert". Tests that are
scraped from the addons documentation are all named test.js, making it
hard to decipher what test is running when only the filename is printed.

Fixes: #6651

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

@bnoordhuis bnoordhuis added the test Issues and PRs related to the tests. label May 9, 2016
@Fishrock123
Copy link
Contributor

idea sgtm but won't review python XD

@gibfahn
Copy link
Member

gibfahn commented May 9, 2016

@bnoordhuis Does the node-test-commit not use the new tools/test.py? Seems to still be test.js for the addons.

https://ci.nodejs.org/job/node-test-commit-linux/nodes=ubuntu1404-64/3282/console

@bnoordhuis
Copy link
Member Author

It's possible the buildbots aren't actually testing the change in this PR; looking at other runs, they seem to be having connectivity issues. It's working for me locally:

$ make test-ci
# <snip>
/usr/bin/python tools/test.py  -p tap --logfile test.tap \
        --mode=release --flaky-tests=run \
         addons doctool known_issues message parallel sequential
1..1121
ok 1 parallel/test-arm-math-exp-regress-1376
  ---
  duration_ms: 0.222
  ...
ok 2 parallel/test-assert
  ---
  duration_ms: 0.342
  ...

@gibfahn
Copy link
Member

gibfahn commented May 9, 2016

Makes sense, your change works manually for me as well.

@bnoordhuis
Copy link
Member Author

@jasnell
Copy link
Member

jasnell commented May 10, 2016

LGTM

@bnoordhuis
Copy link
Member Author

I decided we could do a little better still for nested directories so PTAL. Before, it printed:

ok 1 hello-world/test

Now it prints:

ok 1 addons/hello-world/test

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

@bnoordhuis
Copy link
Member Author

@jasnell Can I ask you to give this a quick look again? The changes since the last review are in the second commit (not that the total diff is that huge: +9 −1.)

@jasnell
Copy link
Member

jasnell commented May 13, 2016

Still LGTM

Print test name as (for example) "parallel/test-assert".  Tests that are
scraped from the addons documentation are all named test.js, making it
hard to decipher what test is running when only the filename is printed.

Fixes: nodejs#6651
PR-URL: nodejs#6653
Reviewed-By: James M Snell <[email protected]>
@bnoordhuis bnoordhuis closed this May 13, 2016
@bnoordhuis bnoordhuis deleted the fix6651 branch May 13, 2016 21:40
@bnoordhuis bnoordhuis merged commit 084b2ec into nodejs:master May 13, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
Print test name as (for example) "parallel/test-assert".  Tests that are
scraped from the addons documentation are all named test.js, making it
hard to decipher what test is running when only the filename is printed.

Fixes: #6651
PR-URL: #6653
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2016
Print test name as (for example) "parallel/test-assert".  Tests that are
scraped from the addons documentation are all named test.js, making it
hard to decipher what test is running when only the filename is printed.

Fixes: #6651
PR-URL: #6653
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2016
Print test name as (for example) "parallel/test-assert".  Tests that are
scraped from the addons documentation are all named test.js, making it
hard to decipher what test is running when only the filename is printed.

Fixes: #6651
PR-URL: #6653
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 18, 2016
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request May 22, 2016
Commit 084b2ec ("test: include component in tap output") introduced
an in hindsight glaringly obvious but fortunately not very critical
Windows-specific bug by failing to take the path separator into account.
This commit rectifies that, the prefix is now correctly stripped.

PR-URL: nodejs#6915
Refs: nodejs#6653
Reviewed-By: Anna Henningsen <[email protected]>
Fishrock123 pushed a commit that referenced this pull request May 23, 2016
Commit 084b2ec ("test: include component in tap output") introduced
an in hindsight glaringly obvious but fortunately not very critical
Windows-specific bug by failing to take the path separator into account.
This commit rectifies that, the prefix is now correctly stripped.

PR-URL: #6915
Refs: #6653
Reviewed-By: Anna Henningsen <[email protected]>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
Commit 084b2ec ("test: include component in tap output") introduced
an in hindsight glaringly obvious but fortunately not very critical
Windows-specific bug by failing to take the path separator into account.
This commit rectifies that, the prefix is now correctly stripped.

PR-URL: #6915
Refs: #6653
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
Commit 084b2ec ("test: include component in tap output") introduced
an in hindsight glaringly obvious but fortunately not very critical
Windows-specific bug by failing to take the path separator into account.
This commit rectifies that, the prefix is now correctly stripped.

PR-URL: #6915
Refs: #6653
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
Commit 084b2ec ("test: include component in tap output") introduced
an in hindsight glaringly obvious but fortunately not very critical
Windows-specific bug by failing to take the path separator into account.
This commit rectifies that, the prefix is now correctly stripped.

PR-URL: #6915
Refs: #6653
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Commit 084b2ec ("test: include component in tap output") introduced
an in hindsight glaringly obvious but fortunately not very critical
Windows-specific bug by failing to take the path separator into account.
This commit rectifies that, the prefix is now correctly stripped.

PR-URL: #6915
Refs: #6653
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Commit 084b2ec ("test: include component in tap output") introduced
an in hindsight glaringly obvious but fortunately not very critical
Windows-specific bug by failing to take the path separator into account.
This commit rectifies that, the prefix is now correctly stripped.

PR-URL: #6915
Refs: #6653
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Commit 084b2ec ("test: include component in tap output") introduced
an in hindsight glaringly obvious but fortunately not very critical
Windows-specific bug by failing to take the path separator into account.
This commit rectifies that, the prefix is now correctly stripped.

PR-URL: #6915
Refs: #6653
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Commit 084b2ec ("test: include component in tap output") introduced
an in hindsight glaringly obvious but fortunately not very critical
Windows-specific bug by failing to take the path separator into account.
This commit rectifies that, the prefix is now correctly stripped.

PR-URL: #6915
Refs: #6653
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants