Bug 205875 – run-webkit-tests: clobber-old-results should remove the entire results folder

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

In automation, we want to clear out the entirety of the results folder when --clobber-old-results is passed. In particular, we want to remove timeout and crash logs.

[Jonathan Bedard]

<rdar://problem/58236117>

[Jonathan Bedard]

Created attachment 387007 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 387007 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387007&action=review

> Tools/ChangeLog:3
> +        run-webki-tests: clobber-old-results should remove the entire results folder

run-webkit-tests

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:-462
> -        # files in the results directory are explicitly used for cross-run
> -        # tracking.

What is this "cross-run tracking"? Is it about saving results for a retry?

Without having thought about the code much, I wonder if the patch makes it so that orginal results will be deleted when using --retry (which is the default).

[Alexey Proskuryakov]

Or when running iPad tests after the main suite.

[Jonathan Bedard]

(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 387007 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387007&action=review
> 
> > Tools/ChangeLog:3
> > +        run-webki-tests: clobber-old-results should remove the entire results folder
> 
> run-webkit-tests
> 
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:-462
> > -        # files in the results directory are explicitly used for cross-run
> > -        # tracking.
> 
> What is this "cross-run tracking"? Is it about saving results for a retry?
> 
> Without having thought about the code much, I wonder if the patch makes it
> so that orginal results will be deleted when using --retry (which is the
> default).

I'm not really sure what this was originally referring to, but I can't see it being anything except undesirable when passing --clobber-old-results.

_clobber_old_results is run before all the rounds, so this doesn't effect results on iOS where we have separate iPhone/iPad rounds.

Another approach we could take its only deleting crashlogs, python stack traces and spindumps instead of the entire directory. The problem with that approach is that I still can't think of a reason why keeping any result data between tests would ever be useful in infrastructure, especially since sometimes we build out of order.

[Jonathan Bedard]

Created attachment 387032 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 387032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387032&action=review

Yes, I also don't remember any files or run-webkit-tests modes where we wanted to keep any files in the results directory between invocations.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:628
> -        # Now we test that --clobber-old-results does remove the old entries and the old retries,
> -        # and that we don't retry again.
> +        # Now we test that --clobber-old-results does remove the old entries and the old retries

Please keep the period at the end of the sentence.

[Jonathan Bedard]

Created attachment 387035 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 387035 [details]
Patch for landing

Clearing flags on attachment: 387035

Committed r254165: <https://trac.webkit.org/changeset/254165>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

Reverted r254165 for reason:

Caused 500+ missing results on Mac

Committed r254176: <https://trac.webkit.org/changeset/254176>

[Jonathan Bedard]

I guess we have our answer. We are using the previous test results to detect when certain tests (compositing, in particular) change their output. This seems like a very unstable way of doing things.

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #12)
> I guess we have our answer. We are using the previous test results to detect
> when certain tests (compositing, in particular) change their output. This
> seems like a very unstable way of doing things.

Probably would have helped to actually delete the right thing.

The previous patch was deleting WebKit/LayoutTests instead of WebKit/build/*/layout-test-results.

[Jonathan Bedard]

Created attachment 387134 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 387134 [details]
Patch

Clearing flags on attachment: 387134

Committed r254235: <https://trac.webkit.org/changeset/254235>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Aakash Jain]

This seems to have broken the layout-tests completely.

https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20%28Tests%29/builds/18157

https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/9327

17:22:50.857 39532 Stopping HTTP server ...
17:22:50.859 39532 Attempting to shut down httpd server at pid 39550

ServerError raised: Failed to stop httpd: httpd: Could not open configuration file /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/httpd.conf: No such file or directory
 (exit code=None)
Traceback (most recent call last):
  File "/Volumes/Data/slave/mojave-release-tests-wk2/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 91, in main
    run_details = run(port, options, args, stderr)
  File "/Volumes/Data/slave/mojave-release-tests-wk2/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 485, in run
    run_details = manager.run(args)
  File "/Volumes/Data/slave/mojave-release-tests-wk2/build/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 314, in run
    self._runner.stop_servers()
  File "/Volumes/Data/slave/mojave-release-tests-wk2/build/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py", line 230, in stop_servers
    self._port.stop_http_server()
  File "/Volumes/Data/slave/mojave-release-tests-wk2/build/Tools/Scripts/webkitpy/port/base.py", line 1063, in stop_http_server
    self._http_server.stop()
  File "/Volumes/Data/slave/mojave-release-tests-wk2/build/Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py", line 134, in stop
    self._stop_running_server()
  File "/Volumes/Data/slave/mojave-release-tests-wk2/build/Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py", line 209, in _stop_running_server
    raise self._server_error('Failed to stop %s' % self._name, err, retval)
ServerError: Failed to stop httpd: httpd: Could not open configuration file /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/httpd.conf: No such file or directory
 (exit code=None)

[WebKit Commit Bot]

Re-opened since this is blocked by bug 205981

[Alexey Proskuryakov]

Next time, please wait for EWS.

[Jonathan Bedard]

Sorry about that....we apparently clobber results after starting the HTTP server.

[Jonathan Bedard]

Created attachment 387232 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 387232 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387232&action=review

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:460
> +        self._printer.write_update("Clobbering all results in {}".format(self._results_directory))

I think that you can just as well say "deleting directory" now, as it's no longer about just results.

[Jonathan Bedard]

Created attachment 388941 [details]
Patch

[Jonathan Bedard]

Created attachment 388945 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 388945 [details]
Patch

Clearing flags on attachment: 388945

Committed r255240: <https://trac.webkit.org/changeset/255240>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.