Skip to content

Conversation

@ErickWendel
Copy link
Member

No description provided.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-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 ErickWendel force-pushed the fix/todo-and-skip-on-spec-reporter branch from b475dd8 to cecf5df Compare April 12, 2023 14:33
@ErickWendel
Copy link
Member Author

ErickWendel commented Apr 12, 2023

@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

@ErickWendel ErickWendel self-assigned this Apr 12, 2023
@cjihrig
Copy link
Contributor

cjihrig commented Apr 12, 2023

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

@pulkit-30
Copy link
Contributor

pulkit-30 commented Apr 12, 2023

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

Also, tests are failing locally.

ArrayPrototypePush,
ArrayPrototypeShift,
} = primordials;
const console = require('console')
Copy link
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 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);
Copy link
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':
return this.#handleTestReportEvent(type, data);
case 'test:skip':
return this.#handleTestReportEvent(type, data);
case 'test:todo':
return this.#handleTestReportEvent(type, data);
case 'test:pass':
case 'test:skip':
case 'test:todo':
return this.#handleTestReportEvent(type, data);

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) {
Copy link
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
Copy link
Contributor

pulkit-30 commented Apr 12, 2023

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
Copy link
Member Author

ErickWendel commented Apr 13, 2023

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

Also, tests are failing locally.

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

@MoLow
Copy link
Member

MoLow commented Apr 13, 2023

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
Copy link
Member

MoLow commented Apr 16, 2023

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

@ErickWendel
Copy link
Member Author

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

gonna check it out!

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants