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: fix tap parser fails if a test logs a number #46056

Merged
merged 8 commits into from
Feb 6, 2023

Conversation

pulkit-30
Copy link
Contributor

@pulkit-30 pulkit-30 commented Jan 2, 2023

fixes: #46048

code: test.mjs

import test from 'node:test';

test('test 1');

test('testing', () => {
  console.log(-123);
  console.log(123);
  console.log('-123');
  console.log('123');
  console.log('a 123');
  console.log(123, '123');
  console.log('123', 123);
  console.log('123', 'a');
  console.log('123 abc');
  console.log('abc 123');
});

test('test 3');

Before changes:

TAP version 13
# Subtest: /Users/pulkitgupta/Desktop/node/test.mjs
not ok 1 - /Users/pulkitgupta/Desktop/node/test.mjs
  ---
  duration_ms: 40.977208
  failureType: 'uncaughtException'
  error: "Cannot read properties of undefined (reading 'value')"
  code: 'ERR_TEST_FAILURE'
  stack: |-
    #Plan (node:internal/test_runner/tap_parser:653:20)
    #TAPDocument (node:internal/test_runner/tap_parser:552:28)
    #parseChunk (node:internal/test_runner/tap_parser:274:35)
    #parseTokens (node:internal/test_runner/tap_parser:249:23)
    TapParser.parse (node:internal/test_runner/tap_parser:105:24)
    TapParser._transform (node:internal/test_runner/tap_parser:187:10)
    Transform._write (node:internal/streams/transform:175:8)
    writeOrBuffer (node:internal/streams/writable:392:12)
    _write (node:internal/streams/writable:333:10)
    Writable.write (node:internal/streams/writable:337:10)
  ...
1..1
# tests 1
# pass 0
# fail 1
# cancelled 0
# skipped 0
# todo 0
# duration_ms 52.074334

After changes:

TAP version 13
# Subtest: /Users/pulkitgupta/Desktop/node/test.mjs
    # -123
    # 123
    # -123
    # 123
    # a 123
    # 123 123
    # 123 123
    # 123 a
    # 123 abc
    # abc 123
    # Subtest: test 1
    ok 1 - test 1
      ---
      duration_ms: 0.677875
      ...
    # Subtest: testing
    ok 2 - testing
      ---
      duration_ms: 0.911917
      ...
    # Subtest: test 3
    ok 3 - test 3
      ---
      duration_ms: 0.041959
      ...
    1..3
ok 1 - /Users/pulkitgupta/Desktop/node/test.mjs
  ---
  duration_ms: 52.6225
  ...
1..1
# tests 1
# pass 1
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 54.201375

I tried to fix this bug, let me know if these changes are acceptable or to be done at some other place.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jan 2, 2023
@aduh95
Copy link
Contributor

aduh95 commented Jan 2, 2023

Can you add a test case?

@pulkit-30
Copy link
Contributor Author

Can you add a test case?

Sure.
also fixing failed checks. 🚀

@pulkit-30
Copy link
Contributor Author

Hey @cjihrig @manekinekko,

Have a look at these changes,
By this we are ignoring invalid Tap syntax from parsing & also displaying it as a diagnostic.
Also added test-case for same here.

Copy link
Contributor

@manekinekko manekinekko left a comment

Choose a reason for hiding this comment

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

The fix LGTM. However, I'd recommend moving the fix to the upper-level logic of the parser since we need to catch all possible invalid syntax encountered by each nested parsing rule.

The most common top-level parsing rule is TAPDocument().

lib/internal/test_runner/tap_parser.js Outdated Show resolved Hide resolved
lib/internal/test_runner/tap_parser.js Outdated Show resolved Hide resolved
lib/internal/test_runner/tap_parser.js Outdated Show resolved Hide resolved
lib/internal/test_runner/tap_parser.js Outdated Show resolved Hide resolved
lib/internal/test_runner/tap_parser.js Outdated Show resolved Hide resolved
lib/internal/test_runner/tap_parser.js Outdated Show resolved Hide resolved
@pulkit-30
Copy link
Contributor Author

pulkit-30 commented Jan 10, 2023

Thanks @adhu95 for suggestions,
I think we should add planStart, planToken and planEnd as private member of class TapParser.

Then we don't need to pass planStart and planEnd as an argument to #Plan method...? WDYT?

@aduh95
Copy link
Contributor

aduh95 commented Jan 10, 2023

I think we should add planStart, planToken and planEnd as private member of class TapParser.

Then we don't need to pass planStart and planEnd as an argument to #Plan method...? WDYT?

I don't think it's a good idea, unless I'm missing something it looks like it makes more sense to keep them as arguments.

lib/internal/test_runner/tap_parser.js Outdated Show resolved Hide resolved
lib/internal/test_runner/tap_parser.js Outdated Show resolved Hide resolved
@pulkit-30 pulkit-30 requested review from manekinekko and aduh95 and removed request for manekinekko and aduh95 January 11, 2023 03:17
@MoLow MoLow added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 5, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 5, 2023
@nodejs-github-bot

This comment was marked as outdated.

@pulkit-30
Copy link
Contributor Author

There are some failing checks, is there anything wrong with this Pull Request?
let me know, I will fix them

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@MoLow MoLow requested a review from aduh95 February 6, 2023 13:52
@MoLow MoLow added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 6, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 6, 2023
@nodejs-github-bot nodejs-github-bot merged commit 4c08c20 into nodejs:main Feb 6, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 4c08c20

@manekinekko
Copy link
Contributor

Congratulations @pulkit-30 🎉

@pulkit-30
Copy link
Contributor Author

Congratulations @pulkit-30 🎉

Thanks a lot @manekinekko for your support and reviews 😇

MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 7, 2023
PR-URL: nodejs/node#46056
Fixes: nodejs/node#46048
Reviewed-By: Moshe Atlow <[email protected]>
(cherry picked from commit 4c08c20e575a0954fe3977a20e9f52b4980a2e48)
MoLow pushed a commit to nodejs/node-core-test that referenced this pull request Feb 8, 2023
PR-URL: nodejs/node#46056
Fixes: nodejs/node#46048
Reviewed-By: Moshe Atlow <[email protected]>
(cherry picked from commit 4c08c20e575a0954fe3977a20e9f52b4980a2e48)
MoLow pushed a commit to MoLow/node that referenced this pull request Feb 18, 2023
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
MoLow pushed a commit to MoLow/node that referenced this pull request Feb 25, 2023
MoLow pushed a commit to MoLow/node that referenced this pull request Feb 25, 2023
@MoLow MoLow added the backport-open-v18.x Indicate that the PR has an open backport. label Feb 25, 2023
MoLow pushed a commit to MoLow/node that referenced this pull request Feb 25, 2023
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
PR-URL: #46056
Backport-PR-URL: #46839
Fixes: #46048
Reviewed-By: Moshe Atlow <[email protected]>
@juanarbol juanarbol mentioned this pull request Mar 3, 2023
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
PR-URL: #46056
Backport-PR-URL: #46839
Fixes: #46048
Reviewed-By: Moshe Atlow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-open-v18.x Indicate that the PR has an open backport. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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.

Tap parser fails if a test logs a number
6 participants