Bug 231265

Created attachment 440299 [details] Patch

Created attachment 440301 [details] Patch

Note: this also works for the case of a Test marked as failure on the Expectation files, and the patch removing the expectation meanwhile the test still fails. The flag '--skip-failing-tests' that is passed by default (in combination with '--skipped=always') also avoid running any tests marked as failing on the Expectation files even when those are passed as arguments.

Comment on attachment 440301 [details] Patch Would like to see if Aakash has any comments, but this looks reasonable to me.

Alexey can comment better about the corner case and --skipped=always, as he was the one to point it out in https://bugs.webkit.org/show_bug.cgi?id=219500#c11. The patch seems fine from code perspective, as in it seems to be doing what it claims to do.

I think that this correctly deals with the specific case that I suggested, however it's not as clear that all cases where TestExpectations are modified are now OK. Perhaps the best way to move forward is if you could list all the interesting cases and why they are OK. A couple cases that come to mind are "Skip" expectation for a directory, and a patch that adds/removes a "Pass Failure" expectation (which is effectively Skip for EWS because of --skip-failing-tests). That would help us organize the thinking around what could be missing. Also, I see that this patch doesn't add a regression test for what would have been wrong if the check for TestExpectations was removed without adding --skipped=always. Could you please add the test?

<rdar://problem/84171814>

(In reply to Alexey Proskuryakov from comment #6) > I think that this correctly deals with the specific case that I suggested, > however it's not as clear that all cases where TestExpectations are modified > are now OK. > > Perhaps the best way to move forward is if you could list all the > interesting cases and why they are OK. A couple cases that come to mind are > "Skip" expectation for a directory, and a patch that adds/removes a "Pass > Failure" expectation (which is effectively Skip for EWS because of > --skip-failing-tests). > > That would help us organize the thinking around what could be missing. > > Also, I see that this patch doesn't add a regression test for what would > have been wrong if the check for TestExpectations was removed without adding > --skipped=always. Could you please add the test? This are all the corner cases that I can think about: 1. A patch that removes an expectation for a test that is Skipped 2. Like above, but for a whole directory 3. A patch that removes an expectation for a test that is marked as Flaky like in "[ Failure Pass]" 4. Like above, but for a whole directory 5. A patch that marks a tests as passing because the directory where it lives is Skipped (or marked as Fail) but this test fails So to test this I made this quick patch modifying the expectation files to test the cases: http://sprunge.us/6Dkoju Then I run WTR passing a list of the tests affected (for all the 5 cases), on the command-line like this: Tools/Scripts/run-webkit-tests --release --skip-failing-tests --skipped=always compositing/contents-format/ipad/deep-color-backing-store.html compositing/ios/basic-layer-properties.html compositing/ios/overflow-scroll-touch-tiles.html compositing/ios/overflow-scroll-update-overlap.html compositing/ios/rasterization-scale.html compositing/ios/rtl-overflow-scrolling-2.html imported/w3c/web-platform-tests/service-workers/service-worker/clients-matchall-frozen.https.html inspector/debugger/stepping/stepNext.html inspector/debugger/stepping/stepping-switch.html webaudio/AudioBufferSource/audiobuffersource-detune-modulation.html webaudio/AudioBufferSource/audiobuffersource-playbackrate-modulation.html And I get the expected behaviour: - With the test patch it runs the 11 tests (some fail/crash/timeout etc) - Without the test patch (clean tree) it skips the 11 tests even when those are specified on the command-line It says: > Found 11 tests; running 0, skipping 11. > All tests skipped. Do you have an idea of other corner cases that can be interesting to test?

I can't. Would it be possible to write regression tests for these cases? Relying on thinking every time is dangerous.

Created attachment 441671 [details] Patch Add regression unit tests for the corner cases

Created attachment 441672 [details] Patch typo fix

Comment on attachment 441672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441672&action=review > Tools/ChangeLog:19 > + * Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py: These are run-webkit-tests tests, not EWS behavior tests, which is what is most relevant to confirm. Can that be tested?

(In reply to Alexey Proskuryakov from comment #12) > Comment on attachment 441672 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=441672&action=review > > > Tools/ChangeLog:19 > > + * Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py: > > These are run-webkit-tests tests, not EWS behavior tests, which is what is > most relevant to confirm. Can that be tested? The EWS behavior is already being tested. It already checks that both the parameters '--skipped=always' and '--skip-failing-tests' are passed on the step of 'run-layout-tests-without-patch' on the unit test test_success_retry_only_subset() Other than the above, this patch doesn't change the EWS behaviour further. What allows this to work is how run-webkit-tests behaves when the flags '--skipped=always' and '--skip-failing-tests' are passed and I also added unit tests for that. So I don't know what more tests I can add.

One way to think about this is: you are changing CISupport/ews-build/steps.py, and are not adding any tests at all that execute that new code. As an example, look at the original concern: > Consider a patch that removes a [ Skip ] test expectation. If the test still fails in reality, then EWS will be green anyway! That's because all three runs will see it fail - passing the test on command line overrides [ Skip ]. Skipped directories would result in something similar, but I'm not even entirely sure what will happen. This is what the test should check, that EWS doesn't provide a wring result in this situation.

(In reply to Alexey Proskuryakov from comment #14) > One way to think about this is: you are changing > CISupport/ews-build/steps.py, and are not adding any tests at all that > execute that new code. > Sorry, but I think this is wrong. I'm modifying the unit test test_success_retry_only_subset() at Tools/CISupport/ews-build/steps_unittest.py to ensure it checks that webkit-test-runner receives the flags '--skipped=always' and '--skip-failing-tests' > As an example, look at the original concern: > > > Consider a patch that removes a [ Skip ] test expectation. If the test still fails in reality, then EWS will be green anyway! That's because all three runs will see it fail - passing the test on command line overrides [ Skip ]. Skipped directories would result in something similar, but I'm not even entirely sure what will happen. > > This is what the test should check, that EWS doesn't provide a wring result > in this situation. And this is tested in the unit-tests test_ews_corner_case_skipped_test() that I added on Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py It checks that when you pass a test on the command line in combination with '--skipped=always' that test won't run

What I mean is, this feature needs tests that verify whether EWS succeeds or fails, and if it fails, which tests it reports as failing.

Comment on attachment 441672 [details] Patch Discussed in Slack, I think that we are on the same page now.

Created attachment 441773 [details] Patch add a few more tests in steps_unittest for the EWS

Created attachment 441775 [details] Patch

Comment on attachment 441775 [details] Patch I think that test coverage is adequate now. I haven't reviewed all of the code line by line, so it may be good to give Aakash and others some time to comment.

Had a quick look, it look good to me.

Thanks for the review! I think enough time has already passed for anyone else to comment further on this. Landing now

Tools/Scripts/svn-apply failed to apply attachment 441775 [details] to trunk. Please resolve the conflicts and upload a new patch.

Committed r285496 (244020@main): <https://commits.webkit.org/244020@main>