Add testsNotRan to keep count of testsNotRan to ensure moduleDone is called #1417
Conversation
step2yeung
force-pushed
the
moduleDoneFix
branch
2 times, most recently
from
4360c16
to
047da76
Compare
step2yeung
force-pushed
the
moduleDoneFix
branch
2 times, most recently
from
7afb00a
to
21571b7
Compare
|
could 'testsNotRan' be changed to 'testsNotRun' to match 'testsRun'? It's inconsistent as it is now. |
step2yeung
force-pushed
the
moduleDoneFix
branch
from
21571b7
to
29b4b7d
Compare
|
@davecombs updated. |
step2yeung
force-pushed
the
moduleDoneFix
branch
from
29b4b7d
to
d4dd246
Compare
step2yeung
force-pushed
the
moduleDoneFix
branch
from
d4dd246
to
e1afd9e
Compare
step2yeung
force-pushed
the
moduleDoneFix
branch
from
e1afd9e
to
a5ec614
Compare
|
Finally got a chance to get back to this and got a simple fix to some of the problem I ran into previously. One important note: |
…filters Previously, the `suiteEnd` event and the `moduleDone` callback were fired when we've run all the tests, based on counting the number of registered tests in a module and the number of tests actually run. This logic does not hold up when there are active module/test filters used in the runner, because then some of the registered tests will never run. The added tests demonstrate the issue, as they will fail without the src patch. Several of the `suiteEnd` events would not fire. The impact of this bug was that the `suiteEnd` event and `moduleDone` callback don't fire if there were active filters such as `--filters` in the CLI or via the "Filter" field in the HTML Reporter. This in turn meant that reporters such as TAP would sometimes reflect the structure of the test suite in an odd way due to it sometimes missing an "end" event for a group of tests. Fixes #1416. Closes #1417.
| @@ -81,14 +81,10 @@ QUnit.module( "after (skip)", { | |||
| } | |||
| } ); | |||
|
|
|||
| QUnit.test( "does not run after initial tests", function( assert ) { | |||
There was a problem hiding this comment.
I'm not sure that this is correct. Looking at it locally, this means the after hook never runs, which would be a regression. During the first/last actually running test, it previously checked if we're currently executing the last non-ignored test. Which meant that if the last registered test is skipped, the after hook will run correctly after the last actually run test.
However, that logic was removed as part of this patch, and thus we now get a similar bug but affecting the after hook rather than the moduleDone callback.
I think for this to work we need to continue to "look ahead" like it did before, so that it knows during this test that it will be the last.
There was a problem hiding this comment.
Agreed, good catch @Krinkle! Let me see what I can do
There was a problem hiding this comment.
@step2yeung Friendly ping to check if you're able to find time for this. If not, I might give it a go next week for the next patch release. Thanks again!
There was a problem hiding this comment.
Thanks for the reminder Krinkle! Will get back to it this week!
Fixes #1416
This PR adds
testsNotRanto a module. This is used to keep track of any tests whereisValid()returns false.This attribute will be used in this condition:
checking for whether the module has completed running its tests.