Bug 231265 – [EWS] Allow the optimization of running only the subset of failed tests on run-layout-tests-without-patch also for patches modifying the TestExpectations files

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

On bug 219500 an optimization for run-layout-tests-without-patch was implemented, but this optimization was avoided for the following corner case:

> 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 ].

However, instead of avoiding the optimization in this case we can pass the flag '--skipped=always' to run-webkit-tests.

That way the test will not be run if it is marked as skipped, even when it is specified on the command-line. So it is safe to apply the optimization in that case as well.

[Carlos Alberto Lopez Perez]

Created attachment 440299 [details]
Patch

[Carlos Alberto Lopez Perez]

Created attachment 440301 [details]
Patch

[Carlos Alberto Lopez Perez]

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.

[Jonathan Bedard]

Comment on attachment 440301 [details]
Patch

Would like to see if Aakash has any comments, but this looks reasonable to me.

[Aakash Jain]

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.

[Alexey Proskuryakov]

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?

[Radar WebKit Bug Importer]

<rdar://problem/84171814>

[Carlos Alberto Lopez Perez]

(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?

[Alexey Proskuryakov]

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

[Carlos Alberto Lopez Perez]

Created attachment 441671 [details]
Patch

Add regression unit tests for the corner cases

[Carlos Alberto Lopez Perez]

Created attachment 441672 [details]
Patch

typo fix

[Alexey Proskuryakov]

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?

[Carlos Alberto Lopez Perez]

(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.

[Alexey Proskuryakov]

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.

[Carlos Alberto Lopez Perez]

(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

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Comment on attachment 441672 [details]
Patch

Discussed in Slack, I think that we are on the same page now.

[Carlos Alberto Lopez Perez]

Created attachment 441773 [details]
Patch

add a few more tests in steps_unittest for the EWS

[Carlos Alberto Lopez Perez]

Created attachment 441775 [details]
Patch

[Alexey Proskuryakov]

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.

[Aakash Jain]

Had a quick look, it look good to me.

[Carlos Alberto Lopez Perez]

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

[EWS]

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

[Carlos Alberto Lopez Perez]

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