Skip to content

test_runner: fix todo and skip messages on spec-reporter - #47525

Closed
ErickWendel wants to merge 1 commit into
nodejs:mainfrom
ErickWendel:fix/todo-and-skip-on-spec-reporter
Closed

test_runner: fix todo and skip messages on spec-reporter#47525
ErickWendel wants to merge 1 commit into
nodejs:mainfrom
ErickWendel:fix/todo-and-skip-on-spec-reporter

Conversation

@ErickWendel

@ErickWendelErickWendel commented Apr 12, 2023

Copy link
Copy Markdown
Member

No description provided.

@nodejs-github-bot

nodejs-github-bot commented Apr 12, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 12, 2023
@ErickWendel
ErickWendelforce-pushed the fix/todo-and-skip-on-spec-reporter branch from b475dd8 to cecf5dfCompareApril 12, 2023 14:33
@ErickWendel

ErickWendel commented Apr 12, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

@MoLow@cjihrig hey there! Do you know why my changes are not being affected when running it with the --test flag?

Without it, the result is:

./node erick-test/index.test.js
▶ my test
□ my todo (0.186458ms)
﹣ my skip (0.069583ms) # SKIP
✔ hello world (0.042875ms)
▶ my test (1.454833ms)
ℹ tests 3
ℹ suites 1
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 1
ℹ todo 1
ℹ duration_ms 7.476542

with it is:

./node --test erick-test/index.test.js
▶ my test
▶ my todo
▶ my skip
✔ hello world (0.03225ms)
✔ my test (1.0185ms)
ℹ tests 1
ℹ suites 1
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 65.784083

@ErickWendelErickWendel self-assigned this Apr 12, 2023
@cjihrig

cjihrig commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

My guess is this is a bug in the TAP parser.

@pulkit-30

pulkit-30 commented Apr 12, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

I think, we have to add cases for test:skip and test:todohere also.

Also, tests are failing locally.

pulkit-30
pulkit-30 reviewed Apr 12, 2023
View reviewed changes
Comment threadlib/internal/test_runner/tests_stream.js
ArrayPrototypePush,
ArrayPrototypeShift,
} = primordials;
const console = require('console')

@pulkit-30pulkit-30Apr 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please remove this.

Comment threadlib/internal/test_runner/reporter/spec.js
Comment on lines 116 to +121
case 'test:pass':
return this.#handleTestReportEvent(type, data);
case 'test:skip':
return this.#handleTestReportEvent(type, data);
case 'test:todo':
return this.#handleTestReportEvent(type, data);

@pulkit-30pulkit-30Apr 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
case'test:pass':
returnthis.#handleTestReportEvent(type,data);
case'test:skip':
returnthis.#handleTestReportEvent(type,data);
case'test:todo':
returnthis.#handleTestReportEvent(type,data);
case'test:pass':
case'test:skip':
case'test:todo':
returnthis.#handleTestReportEvent(type,data);

Comment threadlib/internal/test_runner/tests_stream.js
Comment on lines +35 to +43
skip(nesting, file, testNumber, name, details, directive) {
this.#emit('test:skip', { __proto__: null, name, nesting, file, testNumber, details, ...directive });
}

todo(nesting, file, testNumber, name, details, directive) {
this.#emit('test:todo', { __proto__: null, name, nesting, file, testNumber, details, ...directive });
}

ok(nesting, file, testNumber, name, details, directive) {

@pulkit-30pulkit-30Apr 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please fix indentation

@pulkit-30

pulkit-30 commented Apr 12, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Also, here what I found while debugging this,
Here if you log node.reason, its value is empty string i.e, ''. which is passed to getSkip function which evaluate and return object as [Object: null prototype] { skip: '' }, and later this evaluate skippedSubtest value to undefined and thats why we don't see expected skip logs with --test flag.

if your try to pass args to getSkip function as getSkip(node.reason || undefined), you can see expected behaviour.

@ErickWendel

ErickWendel commented Apr 13, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

I think, we have to add cases for test:skip and test:todohere also.

Also, tests are failing locally.

this is a draft my friend! Not supposed to be reviewed yet

@MoLowMoLow mentioned this pull request Apr 13, 2023
Merged
@MoLow

MoLow commented Apr 13, 2023

Copy link
Copy Markdown
Member

hey there! Do you know why my changes are not being affected when running it with the --test flag?
I opened a PR fixing it: #47537

also do we really need new test:skip and test:todo events?

@MoLow

MoLow commented Apr 16, 2023

Copy link
Copy Markdown
Member

@ErickWendel this should now work (after rebasing) the same with and without --test

@ErickWendel

ErickWendel commented Apr 25, 2023

Copy link
Copy Markdown
MemberAuthor

@ErickWendel this should now work (after rebasing) the same with and without --test

gonna check it out!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@pulkit-30pulkit-30pulkit-30 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@ErickWendelErickWendel

Labels

lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ErickWendel@nodejs-github-bot@cjihrig@pulkit-30@MoLow