Demonstrate what future async support in QUnit should do #583
Conversation
|
I see most of complexity here also related to our reporter, its output is not async friendly. Beyond the output, I think #374 is exactly what we need to fix the leak. Even though, I don't think we need to worry to much with stop/start before refactoring the Assertion object, IMHO, I would check how they would be necessary after this. |
| assert.expect( 1 ); | ||
| setTimeout(function() { | ||
| assert.ok( true, "this belongs to test #1" ); | ||
| }); |
There was a problem hiding this comment.
I know this is an example of assertion leaking to other tests, but in any other situation I would say this is not exactly intended to work in a sync test block. :)
There was a problem hiding this comment.
Did you see my comment in front of the test? I just kept it simple. If you hadn't seen it, hopefully that makes sense then. If you had, let me know, too, and I will revisit this.
There was a problem hiding this comment.
That's alright, the example is good enough to point the issue we need to fix. Thanks
Honestly, it seems the HTML reporter can be more async friendly than most other reporter types can be: add an initial entry for each started test that shows "Running..." and then update it with the results when it is finished. It's definitely less easy to achieve this favorable effect with async tests while using non-HTML reporters. |
|
@JamesMGreene that's one of the reasons why I'm trying to decouple the HTML reporter in #603. I wanna center all the HTML reporter stuff to then make it async friendly. Doing that, will turn QUnit more friendly to couple other reporters and internal refactorings without the HTML stuff included. Other part of the plan is to make QUnit report to the console when it's available (using tap, probably), so it will be simple enough to plug on many environments without tons of workarounds. |
|
Implementation of this is tracked as #534. |
They were trying to test assertions being assigned to it's own current tests instead of leaking to other tests. Although they proved their bad implementation on qunitjs#583 and they should be implemented on logging tests. Also they're testing the HTML Reporter only.
They were trying to test assertions being assigned to it's own current tests instead of leaking to other tests. Although they proved their bad implementation on qunitjs#583 and they should be implemented on logging tests. Also they're testing the HTML Reporter only.
They were trying to test assertions being assigned to it's own current tests instead of leaking to other tests. Although they proved their bad implementation on qunitjs#583 and they should be implemented on logging tests. Also they're testing the HTML Reporter only.
They were trying to test assertions being assigned to it's own current tests instead of leaking to other tests. Although they proved their bad implementation on qunitjs#583 and they should be implemented on logging tests. Also they're testing the HTML Reporter only.
They were trying to test assertions being assigned to it's own current tests instead of leaking to other tests. Although they proved their bad implementation on qunitjs#583 and they should be implemented on logging tests. Also they're testing the HTML Reporter only.
They were trying to test assertions being assigned to it's own current tests instead of leaking to other tests. Although they proved their bad implementation on qunitjs#583 and they should be implemented on logging tests. Also they're testing the HTML Reporter only.
They were trying to test assertions being assigned to it's own current tests instead of leaking to other tests. Although they proved their bad implementation on qunitjs#583 and they should be implemented on logging tests. Also they're testing the HTML Reporter only.
They were trying to test assertions being assigned to it's own current tests instead of leaking to other tests. Although they proved their bad implementation on qunitjs#583 and they should be implemented on logging tests. Also they're testing the HTML Reporter only.
- Removes QUnit.init and deprecated QUnit.reset - Extends QUnit.extend to accept a `undefOnly` parameter, so we can check if it's needed to extend only unset/undefined properties - HTML Logging is now related to each assertion through QUnit.log - Now it logs exactly on the assertion's related test, without leaking. Ref qunitjs#583 - The HTML reporter was refactored and improved. Ref qunitjs#603
- Removes QUnit.init and deprecated QUnit.reset - Extends QUnit.extend to accept a `undefOnly` parameter, so we can check if it's needed to extend only unset/undefined properties - HTML Logging is now related to each assertion through QUnit.log - Now it logs exactly on the assertion's related test, without leaking. Ref qunitjs#583 - The HTML reporter was refactored and improved. Ref qunitjs#603
|
I believe this can be closed now with #603. I'm not sure if |
|
This works as expected now:
|
|
Though, I wonder: should we fail all assertions made in a test context after the test has finished regardless of their actual boolean result? Or is having them appear after the "Expected 1 assertions, but 0 were run" message enough of a cue? |
|
I think it's enough, this is a user(dev) responsibility to handle async tests properly now. We fixed the leaking, but we can't avoid them from not writing good tests. |
|
I wonder if outputting timestamps for tests and assertions could help identify this oddity, along with helping elsewhere. Where "timestamp" would be time in milliseconds since the test started running. So the As for helping elsewhere: For async tests with multiple timeouts, it might be useful to see where the delays actually are, instead of only seeing the total runtime of a test. If that sounds useful to anyone else, I'll create a separate ticket. |
|
I liked the idea of the timestamps. |
|
Agreed, timestamps relative to the test start sound useful. |
Krinkle
added
Type: Enhancement
Category: Tests
Type: Meta
This is not intended to be merged, at least not as-is. The idea here is to show what the problem with async assertions is, and what it should look and behave like in the future.
Running the testsuite from this PR currently results in this output:
This highlights an important issue with async tests right now: Assertions can leak into any other test. In this case I've constructed deterministic tests. In practice, the result is not deterministic and much worse, because the "async assertion" can end up in any other test.
The desired output here would deal with that. So that test #1 would fail with "Expected 1 assertions, but 0 were run", but also have a failing assertion with stacktrace and the original message "this belongs to test #1", in addition to something like "assertion ran outside of its own test context". The same would apply to test #2, except that the assertion here would fail for two reasons. Both "catchAll" tests would be green, having 0 assertions. This is what #374 is about.
This PR also demonstrates a possible replacement for the current
stop()/start()pair, addingasyncanddonemethods to the assertion argument. As demonstrated here, that's a pretty trivial change, though it might get more interesting when combined with the above or the suggestions regarding promises in #534.@leobalter would be great if you could take a look at this. We can then discuss some details and look into actually implementing it. Maybe @Krinkle has input, too?