test: fix tap parser fails if a test logs a number by pulkit-30 · Pull Request #46056 · nodejs/node · GitHub

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.

Review requested:

@nodejs/test_runner

Can you add a test case?

Can you add a test case?

Sure.
also fixing failed checks. 🚀

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.

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().

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?

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.

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

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

Landed in 4c08c20

Congratulations @pulkit-30 🎉

Congratulations @pulkit-30 🎉

Thanks a lot @manekinekko for your support and reviews 😇

PR-URL: nodejs/node#46056 Fixes: nodejs/node#46048 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> (cherry picked from commit 4c08c20e575a0954fe3977a20e9f52b4980a2e48)

PR-URL: #46056 Fixes: #46048 Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

PR-URL: nodejs#46056 Fixes: nodejs#46048 Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

PR-URL: #46056 Backport-PR-URL: #46839 Fixes: #46048 Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

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

pulkit-30 force-pushed the fix-46048 branch from 7db255e to 486b96f Compare January 2, 2023 10:06

pulkit-30 marked this pull request as draft January 2, 2023 10:11

pulkit-30 force-pushed the fix-46048 branch from 486b96f to 28efd7b Compare January 2, 2023 12:44

pulkit-30 marked this pull request as ready for review January 2, 2023 13:18

pulkit-30 force-pushed the fix-46048 branch from 28efd7b to 74ea047 Compare January 6, 2023 12:38

pulkit-30 force-pushed the fix-46048 branch from 74ea047 to dba8a16 Compare January 8, 2023 05:43

manekinekko reviewed Jan 9, 2023

View reviewed changes

aduh95 reviewed Jan 10, 2023

View reviewed changes

Comment thread lib/internal/test_runner/tap_parser.js Outdated

aduh95 reviewed Jan 10, 2023

View reviewed changes

Comment thread lib/internal/test_runner/tap_parser.js Outdated

Comment thread lib/internal/test_runner/tap_parser.js Outdated

pulkit-30 requested review from aduh95 and manekinekko and removed request for aduh95 and manekinekko January 11, 2023 03:17

MoLow approved these changes Feb 5, 2023

View reviewed changes

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 Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 5, 2023

This comment was marked as outdated.

Sign in to view

github-actions Bot mentioned this pull request Feb 6, 2023

CI Reliability 2023-02-06 nodejs/reliability#497

Open

19 tasks

MoLow requested a review from aduh95 February 6, 2023 13:52

MoLow added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 6, 2023

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 merged commit 4c08c20 into nodejs:main Feb 6, 2023

github-actions Bot mentioned this pull request Feb 7, 2023

CI Reliability 2023-02-07 nodejs/reliability#498

Open

18 tasks

MylesBorins pushed a commit that referenced this pull request Feb 18, 2023

test: fix tap parser fails if a test logs a number

89aa161

PR-URL: #46056 Fixes: #46048 Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

MylesBorins mentioned this pull request Feb 19, 2023

v19.7.0 proposal #46725

Merged

MoLow added the backport-open-v18.x label Feb 25, 2023

MoLow pushed a commit to MoLow/node that referenced this pull request Feb 25, 2023

test: fix tap parser fails if a test logs a number

9c49554

PR-URL: nodejs#46056 Fixes: nodejs#46048 Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

juanarbol pushed a commit that referenced this pull request Mar 3, 2023

test: fix tap parser fails if a test logs a number

aac7898

PR-URL: #46056 Backport-PR-URL: #46839 Fixes: #46048 Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

juanarbol mentioned this pull request Mar 3, 2023

V18.15.0 proposal #46920

Merged

juanarbol pushed a commit that referenced this pull request Mar 5, 2023

test: fix tap parser fails if a test logs a number

4f5aff2

PR-URL: #46056 Backport-PR-URL: #46839 Fixes: #46048 Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

test: fix tap parser fails if a test logs a number by pulkit-30 · Pull Request #46056 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: fix tap parser fails if a test logs a number - #46056

test: fix tap parser fails if a test logs a number#46056nodejs-github-bot merged 8 commits intonodejs:mainnodejs/node:mainfrom pulkit-30:fix-46048pulkit-30/node:fix-46048Copy head branch name to clipboard

Conversation

pulkit-30 commented Jan 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 2, 2023

Uh oh!

aduh95 commented Jan 2, 2023

Uh oh!

pulkit-30 commented Jan 2, 2023

Uh oh!

pulkit-30 commented Jan 6, 2023

Uh oh!

manekinekko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pulkit-30 commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Jan 10, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

pulkit-30 commented Feb 5, 2023

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Feb 6, 2023

Uh oh!

nodejs-github-bot commented Feb 6, 2023

Uh oh!

manekinekko commented Feb 6, 2023

Uh oh!

pulkit-30 commented Feb 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

test: fix tap parser fails if a test logs a number#46056
nodejs-github-bot merged 8 commits into
nodejs:mainfrom
pulkit-30:fix-46048

pulkit-30 commented Jan 2, 2023 •
edited
Loading

pulkit-30 commented Jan 10, 2023 •
edited
Loading