-
Notifications
You must be signed in to change notification settings - Fork 783
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
Core: Implement QUnit.todo #1080
Conversation
this looks amazing! It might be a breaking change for the reporters but it's worth a major release for this, which we can bundle with a few other features, including the js-reporters. |
I don't think we need a major version bump. As I said, this doesn't break any existing set ups; it should only cause problems if someone starts adding todo tests. I actually think this gives a fairly good rollout strategy as reporters can be updated over time, rather than expecting them all to do it at once. |
Got it! I'm +1 to merge it as soon we can get the updated the grunt plugin ready. |
it is breaking in the logs file, this is weird. I need to spend a bit more time on this. |
ok, got it. Had to remind what was happening here. I'll check how is the status on grunt-contrib-qunit |
I asked @jzaefferer a little help in there and also write/publish access to the grunt plugin. I'll be able to update it as soon as we solve this. |
sorry for the spam, but I finally got access to it! Thanks @tkellen! |
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 really like this approach; it seems far simpler than tracking things down to the assertion level. A few comments, but overall in favor. 👏
reporter/html.js
Outdated
testItem = id( "qunit-test-output-" + details.testId ); | ||
|
||
assertList = testItem.getElementsByTagName( "ol" )[ 0 ]; | ||
|
||
good = details.passed; | ||
bad = details.failed; | ||
|
||
if ( bad === 0 ) { | ||
let failed = details.todo ? !details.failed : !!details.failed; |
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.
Although logically correct, I find this hard to read because "failed" is used as both a boolean and a boolean-coerced number. It's purely a matter of taste, but I'd go for something like:
// This test passed if it has no unexpected failed assertions
let testPassed = details.failed > 0 ? details.todo : !details.todo;
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.
Good suggestion. I'll updated this and dependent logic.
reporter/html.js
Outdated
let todoLabel = document.createElement( "em" ); | ||
todoLabel.className = "qunit-todo-label"; | ||
todoLabel.innerHTML = "todo"; | ||
testItem.insertBefore( todoLabel, testTitle ); |
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.
Could we separate this presentation logic from the data updates?
if ( details.todo ) {
let todoLabel = document.createElement( "em" );
todoLabel.className = "qunit-todo-label";
todoLabel.innerHTML = "todo";
testItem.insertBefore( todoLabel, testTitle );
}
…
if ( failed ) {
stats.failedTests++;
} else if ( todo ) {
stats.todoTests++;
} else {
stats.passedTests++;
}
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.
Yep, this helped simplify logic and keeps things clearer.
reporter/html.js
Outdated
@@ -1,6 +1,14 @@ | |||
import QUnit from "../src/core"; | |||
import { window, navigator } from "../src/globals"; | |||
|
|||
const stats = { | |||
totalTests: 0, |
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.
Could we document the relationships between these values? It's worth commenting that totalTests
equals the sum of the other values, and maybe even keeping two variables (e.g., let totalTests = 0; const testPartitions = {…}
).
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'll remove totalTests
from being tracked altogether. We only use it after the run has finished to display the number of tests that ran, so I just created a local variable at that point which clearly shows that it is the sum of the other categories.
build/tasks/test-on-node.js
Outdated
testActive = false; | ||
|
||
if ( details.failed && !details.todo ) { | ||
stats.failedTests++; |
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.
This will fail to increment failedTests
when a TODO test has no failing assertions. I think it should instead use the same logic as the HTML reporter.
Updated with the suggestions from @gibson042 and opened a PR to update grunt-contrib-qunit to support |
build/tasks/test-on-node.js
Outdated
|
||
if ( details.skipped ) { | ||
stats.skipped++; | ||
} else if ( details.failed && !details.todo ) { |
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.
Todo tests can also "fail". I think this would be clearest if it mirrored the HTML reporter:
// This test passed if it has no unexpected failed assertions
var testPassed = details.failed > 0 ? details.todo : !details.todo;
if ( details.skipped ) {
stats.skipped++;
} else if ( !testPassed ) {
stats.failed++;
} else if ( details.todo ) {
stats.todo++;
} else {
stats.passed++;
}
Updated once more. @leobalter when you have a chance, would appreciate your help reviewing and landing this (will require a new grunt-contrib-qunit release first). |
LGTM. We can already include the expected grunt-contrib-qunit 1.3.0 |
26ac494
to
ce025b6
Compare
Updated with the dependency update; tests are now passing! |
reporter/html.js
Outdated
stats.todoTests++; | ||
} else { | ||
stats.passedTests++; | ||
} |
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.
This need to be moved up inside the else
block so skipped tests don't increment two stats
counters.
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.
Good catch.
Updated again. Unless any other issues are found, I think this should be good to land (will open corresponding documentation PR soon) |
|
Corresponding docs PR is now open: qunitjs/api.qunitjs.com#146 @gibson042 if this seems good to you, we can merge (and finally close this feature out!) |
Waiting for @gibson042 to confirm the changes after his feedback, LGTM. |
Supersedes #1006. Original issue #787.
This takes a simpler approach to implementing
QUnit.todo
. We simply add atodo
flag tolog
andtestDone
details that denotes if an assertion or a test was part of atodo
. The reporter can then handle that scenario appropriately.This approach keeps QUnit's internal logic simple and encourages reporters to report success/failure for a test suite based on the outcome of tests and not on the outcome of assertions, which is inline with the thinking of js-reporters.
While this likely means most reporters will have to implement new logic to support
todo
, this change should not break existing reporters and provides a relatively clear path to supporting the new feature.Note: Assuming this implementation looks okay, I will open a PR to support
todo
ingrunt-contrib-qunit
.