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: update tap reporter version to 14 #44041

Closed
wants to merge 3 commits into from
Closed

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Jul 29, 2022

Fixes #44040 according to TAP specification available on http://testanything.org/tap-version-14-specification.html

Example test:

const test = require('node:test');
const assert = require('node:assert');

test('top-level test 1', async (t) => {
  await t.test('level 1.1', () => {});
});

test('top-level test 2', () => {});

Produced output:

TAP version 14
# Subtest: top-level test 1
    ok 1 - level 1.1
      ---
      duration_ms: 0.001925209
      ...
    1..1
ok 1 - top-level test 1
  ---
  duration_ms: 0.002924375
  ...
ok 2 - top-level test 2
  ---
  duration_ms: 0.000044334
  ...
1..2
# tests 2
# pass 2
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 0.031293292

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jul 29, 2022
@MoLow
Copy link
Member

MoLow commented Jul 29, 2022

How well does this play with tap parsers such as https://www.npmjs.com/package/tap-mocha-reporter ?
It was originally introduced since tap parsers did not relate to nested tests as nested
See
#43417

@anonrig
Copy link
Member Author

anonrig commented Jul 29, 2022

Thanks for the question @MoLow. Here's the output using tap-mocha-reporter

➜  node git:(fix/tap-v14) ✗ ./out/Release/node ./tap-test.js | tap-mocha-reporter spec
(node:66047) ExperimentalWarning: The test runner is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

top-level test 1
  ✓ level 1.1
  ✓ top-level test 2

  2 passing (5ms)

@manekinekko
Copy link
Contributor

manekinekko commented Jul 29, 2022

@MoLow here is a screenshot comparing all 3 scenarios:

  1. current output (test_runner: update output TAP format to follow TAP 14 specs #44040)
  2. node-tap
  3. changes from this PR

I think we are good! WDYT?

image

@manekinekko manekinekko mentioned this pull request Jul 29, 2022
8 tasks
@MoLow
Copy link
Member

MoLow commented Jul 29, 2022

@MoLow here is a screenshot comparing all 3 scenarios:

  1. current output (test_runner: update output TAP format to follow TAP 14 specs #44040)
  2. node-tap
  3. changes from this PR

I think we are good! WDYT?

image

Well only the first example looks correct to me.
Top level test 2 should not be under top level test 1, or am I missing anything?

@manekinekko
Copy link
Contributor

manekinekko commented Jul 29, 2022

@anonrig specs 14 state that subtests need to be terminated by a test point.

TAP version 14
# Subtest: subtest name      <<< start of subtest
     ...
ok 1 - subtest name          <<< end of subtest (could be ok or not ok)

@anonrig
Copy link
Member Author

anonrig commented Jul 29, 2022

Actually this is related to Tap 14 and how tap-mocha-reporter does not handle tap 14 grammar correctly. If we want to support tap 14 we need to ignore this, since it is not backward compatible according to the specification

Since TAP13 specifies that non-TAP output should be ignored or provided directly to the user, and indented Test Points and Plans are non-TAP according to TAP13, only the terminating correlated test point will be interpreted by most TAP13 Harnesses. Thus, they will usually report the overall subtest result correctly, even if they lack the details about the results of the subtest

@manekinekko
Copy link
Contributor

Thanks for investigating this @anonrig! I agree we should ignore the mocha report case (unless there is a requirement I am not aware of).

@MoLow the upcoming built-in TAP parser (#43525) is built on top of TAP 14. In most cases, TAP 14 is backward compatible with TAP13. I suggest we focus on TAP 14 and then later add TAP 13 specifics (if needed).

@aduh95
Copy link
Contributor

aduh95 commented Jul 29, 2022

/cc @nodejs/test_runner

@MoLow
Copy link
Member

MoLow commented Jul 30, 2022

@manekinekko can the built-in parser support both TAP 13 and TAP 14?
I think until we have a built-in solution for human-readable output, we should try and support the human-readable use case with ecosystem packages such as tap-mocha-reporter work

@manekinekko
Copy link
Contributor

I guess the answer is yes. The parser will need to implement both strategies. The current implementation will need some major refactoring but it's manageable.

However, the current TAP 14 support is not 100% complete. I'd say we are 95%. Local pragmas support needs some twerking. Diagnostic data is parsed as raw data (not yaml).

@cjihrig
Copy link
Contributor

cjihrig commented Jul 31, 2022

Local pragmas support needs some twerking.

Sorry for this unproductive comment, but I hope you meant to say tweaking 😄

@anonrig
Copy link
Member Author

anonrig commented Jul 31, 2022

Should we produce different tap output according to different flags? What is the best alternative for producing 2 (or more) different tap versions for the test runner?

@manekinekko
Copy link
Contributor

Local pragmas support needs some twerking.

Sorry for this unproductive comment, but I hope you meant to say tweaking 😄

OMG!! Sorry about the typo. I definitely meant "tweaking" 😂

@manekinekko
Copy link
Contributor

Should we produce different tap output according to different flags? What is the best alternative for producing 2 (or more) different tap versions for the test runner?

According to specs, TAP 14 must be backwards compatible with TAP 13.

@anonrig
Copy link
Member Author

anonrig commented Aug 2, 2022

I reverted my subtest print changes and just left the tap version print change and updated the tests. If there are any other changes required, I'll be happy to resolve @manekinekko

@ErickWendel
Copy link
Member

@anonrig it seems some tests are broken now, would you mind checking it?

@anonrig
Copy link
Member Author

anonrig commented Aug 8, 2022

Updated the last commit and forced pushed with the unhandled test case pointing to version 13. @ErickWendel

@anonrig anonrig closed this Oct 15, 2022
@mattfysh
Copy link

@manekinekko apologies for the OTT - how are you getting tap-mocha-reporter working with the node test runner output?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_runner: update output TAP format to follow TAP 14 specs
8 participants