Bug 63844

It's actually somewhat worse than that, *anything* in layout-test-results can get overwritten when re-run (e.g., the -actual.* files get clobbered, stacks get clobbered, etc.). At one point Ojan and I talked about changing the layout-test-results directory if we cared enough about this, so that you got a layout-test-results.retries output dir or something, but we didn't care enough :) I'm not sure if there's a better solution.

I'm fine with adding a "retries" sub-directory which is used for the re-run.

This is a polish bug.

This fix would be great to help debugging DRT sideeffect bugs revealed by NRWT. Kristóf, could you pick up this bug?

Created attachment 114270 [details] draft patch Hi, I'am working on these bug. Do you have any idea to make it better?

Comment on attachment 114270 [details] draft patch Patch looks good to me. Needs tests though. Another way this could be implemented is to flip a "retrying" flag on the port object and then port.results_directory() could return the retries directory. I don't know if that's cleaner or more magic though. I defer to Eric or Dirk here. R- just for the lack of tests.

Comment on attachment 114270 [details] draft patch I agree with Ojan that this patch definitely needs some tests, but otherwise it looks okay. Nice job! I was tempted to say that we should just use options.retrying instead of passing another flag around, but I didn't like the idea of values in the options object changing after startup. I was also tempted by adding a flag on the port object to tell it to change what results_directory() returned (as Ojan suggested), but (a) that felt like a layering violation, (b) doesn't really help since you need to propagate the 'retrying' flag across the process boundary for the multiprocessing case, since we have to create a new port object in the worker processes. So this feels like the way to go short of creating an arg class that gets passed to the workers that encapsulates worker_number, retrying, and options. But we can do that if there's a next time.

We could also just check the filesystem and see if the file exists and never overwrite. Instead write with an appended number: http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/system/workspace.py#L46 Then we just have to be sure to clear the directory before. I'm not sure that's a better solution, but it is an option.

(In reply to comment #8) > We could also just check the filesystem and see if the file exists and never overwrite. Instead write with an appended number: > http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/system/workspace.py#L46 > > Then we just have to be sure to clear the directory before. > > I'm not sure that's a better solution, but it is an option. Yeah, I thought of that, too, but I like being able to clearly segregate the tries from the retries.

Oh, but that reminds me ... this patch should ensure that *everything* that is being written under the results_directory is retrying-aware, not just test_runX.txt. I.e., we currently clobber -expected and -actual files as well, and we should stop that.

It seems a much simpler solution would just be to change the results directory during retry attempts. :)

(In reply to comment #11) > It seems a much simpler solution would just be to change the results directory during retry attempts. :) Well, yeah. So I think we need to add a results_directory() wrapper somewhere in the controller code that is retry-aware, and use that instead of calling port.results_directory() directly (so that the port code doeesn't need to be retry-aware).

I'm not sure results_directory is even a Port concern anymore? Do ports specify separate retry directories?

(In reply to comment #13) > I'm not sure results_directory is even a Port concern anymore? Do ports specify separate retry directories? No, that's the point ;) Different ports have different defaults for the results_directory, though. Looking at the code now, though, it looks like chromium defaults to $CHROMIUM_SRC/webkit/$CONFIGURATION/layout-tests-results and other webkit.py-based ports default to `webkit-build-directory`/$CONFIGURATION/layout-test-results. I imagine we could get the two forks to merge without too much hassle, though.

I see, then I will rewrite it, and make a unit test too

Created attachment 115804 [details] proposed fix with unit test

Comment on attachment 115804 [details] proposed fix with unit test View in context: https://bugs.webkit.org/attachment.cgi?id=115804&action=review The manager_worker_broker class shouldn't have to know anything about retrying logic, and modifying a module-level global is bad. I think we can just pass the output directory to use through when we start each worker, and that might end up a lot cleaner. > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:913 > + manager_worker_broker.retry = True I think instead of doing this we can just modify start_worker() to take the output directory to use along with the worker number, and then change the output directory based on whether self._retrying is True. > Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:266 > + def test_retrying(self): In addition to this test, it might be good to make sure that the 'retrying' flag propagates correctly across the process boundary in the multiprocess case (this test will only test the inline case). Unfortunately, you can't test that with the --platform=test class, since that uses a mock filesystem (which isn't shared across process), which means we'd need to test this with a real integration test. Maybe just add a a FIXME for now? > Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:68 > +retry = False This isn't necessary. > Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:109 > +def results_directory(port): This function doesn't belong here; the broker class shouldn't have to know anything about the results_directory. > Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:44 > + root_output_dir = manager_worker_broker.results_directory(port) I suggest we pass in the output directory to use instead. > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:71 > + tests_run_filename = self._filesystem.join(manager_worker_broker.results_directory(self._port), "tests_run%d.txt" % self._worker_number) We should pass in the output directory like we do the worker number.

Created attachment 117193 [details] proposed fix It now save the tests_run*.txt in the [layout_test_dir]/retries, but only these lists, not the entire new test results, because it needs to change the test_results_writer.py and I didn't want to make any ugly hacks in it.

Comment on attachment 117193 [details] proposed fix Looks good enough. Updating the test results and making sure everything else is consistent can build on this. Thanks for doing the work! > Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:126 > + self.results_directory = results_directory Nit: probably should be self._results_directory (note the "_"). > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:71 > + tests_run_filename = self._filesystem.join(self.results_directory, "tests_run%d.txt" % self._worker_number) Nit: self._results_directory again.

Created attachment 117411 [details] proposed fix Thanks for your forbearance

(In reply to comment #20) > Created an attachment (id=117411) [details] > proposed fix > > Thanks for your forbearance The patch was r+ -ed with small modifications, so you don't need one more review. I landed your fixed patch manually http://trac.webkit.org/changeset/101667

(In reply to comment #21) > (In reply to comment #20) > > Created an attachment (id=117411) [details] [details] > > proposed fix > > > > Thanks for your forbearance > > The patch was r+ -ed with small modifications, so you don't need one more review. > > I landed your fixed patch manually http://trac.webkit.org/changeset/101667 Thank you

Comment on attachment 117411 [details] proposed fix clearing r, cq flags.