WIP Report unexpected tests, discovered-but-unrun tests #67
Conversation
mistydemeo
force-pushed
the
warn_skipped_unexpected_suites
branch
2 times, most recently
from
428eee7
to
d0e0bc7
Compare
|
Based on a discussion with @bhuga, I've tweaked discovery to always run even if the stats file contains all of the whitelisted tests. |
|
@mistydemeo Let me know when you're ready for review! |
|
With the tests passing now, I think I'm ready for comments. |
There was a problem hiding this comment.
Nice! It'll be great to have this error checking in place.
What do you think about going a step further and verifying that suites were run on the same worker we assigned them to? If we sent a hostname/pid pair in the POP message then we could keep track of which worker was assigned which suite, then check that against worker.suites at the end. Does that seem worthwhile? It seems like it might get us closer to figuring out exactly how things went wrong when these checks fail.
| @@ -311,13 +338,17 @@ def enqueue_discovered_suite(suite_name, path) | |||
|
|
|||
| if @original_queue.include?([suite_name, path]) | |||
| # This suite was already added to the queue some other way. | |||
| # We still want to track it in the list of discovered suites though. | |||
| @discovered_suites << [suite_name, path] | |||
There was a problem hiding this comment.
I'd move this up to before the if and get rid of the comment. That will get rid of the duplication of this line after the block.
| @@ -162,13 +168,34 @@ def summarize_internal | |||
| puts @failures | |||
| end | |||
|
|
|||
| skipped_suites = @discovered_suites - run_suites | |||
There was a problem hiding this comment.
I'm pretty sure we want to skip all this logic on remote masters (i.e., if relay? is true). Remote masters don't discover suites so @discovered_suites will always be empty, but run_suites will not be empty, so we'll always have "unexpected" suites. (If that turns out to be correct, it'd be great to have a test that verifies this if possible.)
There was a problem hiding this comment.
Good point, I hadn't considered this. Updated.
Can this get weird if a suite is moved from one file to another, and a worker loads a file with an unassigned suite? Or is that all handled nicely? |
In response to
So I don't think we need to worry about loading a file with an unassigned suite; if the suite's name doesn't match it won't run it. |
Alright. Another thing to consider with host tracking with this we'd need to track assigned tests instead of discovered tests. This is a line more complicated, because we can assign tests that don't get run because tests are moved or deleted. So the algorithm needs to be:
|
mistydemeo
force-pushed
the
warn_skipped_unexpected_suites
branch
from
5a3042d
to
2c29245
Compare
|
Updated - it now reports on whether tests were run on the assigned hosts, as well as whether tests were missing or run unexpectedly. |
| puts | ||
|
|
||
| @stats.save | ||
|
|
||
| summarize | ||
|
|
||
| estatus = @completed.inject(0){ |s, worker| s + worker.status.exitstatus } | ||
| estatus += 1 unless @discovered_suites.empty? && misrun_suites.empty? && unassigned_suites.empty? |
There was a problem hiding this comment.
I think unassigned_suites will be non-empty on remote masters because @discovered_suites will always be empty. Seems like maybe more of the work needs to move into the if !relay? block.
|
Updated to ignore that logic on relays. |
This is adapted from a couple of commits in @aroben's suite-files branch. This is definitely still broken, opening a PR to compare cibuild results to my local runs.
In the "rename" test, the queue passed whenRunneris instantiated at the beginning of the second test run contains both the old and new suite names, both associated with the same path. The old suite names are flagged as discovered-but-not-run.A couple of the rspec examples are broken, possibly due to example splitting.