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
[Fix] emit skipped tests as objects #473
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests covering your new changes.
Will add tests for |
Please do; tests help review of code. |
Node 0.8 has a weird issue that I don't understand why only it fails. |
test/todo_explanation.js
Outdated
+ 'not ok 2 needs insight # TODO\n' | ||
+ ' ---\n' | ||
+ ' operator: fail\n' | ||
+ ' at: Test.<anonymous> ($TEST/todo_explanation.js:$LINE:$COL)\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test on 0.8 seems to want Test.tap.test.test.todo
here instead of Test.<anonymous>
:-/
I'm confused because we have many other tests using Test.<anonymous>
that don't have special handling for node 0.8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that, but only why node 0.8 😭 I've gone through the source and debugged it a thousand times but only node 0.8 failes for only todo_explanation.js
. It's just a copy of todo.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if the change to `emit('prerun') somehow changed things for the v8 version 0.8 is using.
We could update test/common
, perhaps, for node 0.8 only, to swap out Test.tap.test.test.todo
for Test.<anonymous>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if the change to `emit('prerun') somehow changed things for the v8 version 0.8 is using.
I tried by using the latest commit from master
, It still has the same problem. It works normally, this PR effects it somehow.
My [naive] understanding is that in V8 version of 0.8, the stack
part assumes the {todo: 'something'}
object as one of the stack layer. The same happens for my local skip_explanation.js
test. The test in #473 (comment) comment has the function in stack pointing to Test.tap.test.platform
.
Update: IDK but when I pass strings as arguments to todo or skip this happens, when they are booleans it totally works.
Removed loose patches. Current changes include:
Will try to add descriptive messages in another PR |
c347a5d
to
8d5dc2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great as a bugfix.
When using
objectMode
skipped tests are emitted as comments, they are actual tests that need to be emitted as objects.Test code:
When used normally:
After patch: