Bug 63844 - tests_run0.txt gets clobbered when re-running failing tests
Summary: tests_run0.txt gets clobbered when re-running failing tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kristóf Kosztyó
URL:
Keywords:
Depends on:
Blocks: 64491
  Show dependency treegraph
 
Reported: 2011-07-01 13:47 PDT by WebKit Review Bot
Modified: 2011-12-01 13:36 PST (History)
7 users (show)

See Also:


Attachments
draft patch (8.10 KB, patch)
2011-11-09 07:13 PST, Kristóf Kosztyó
ojan: review-
Details | Formatted Diff | Diff
proposed fix with unit test (6.86 KB, patch)
2011-11-18 07:20 PST, Kristóf Kosztyó
dpranke: review-
Details | Formatted Diff | Diff
proposed fix (12.38 KB, patch)
2011-11-30 07:44 PST, Kristóf Kosztyó
dpranke: review+
dpranke: commit-queue-
Details | Formatted Diff | Diff
proposed fix (12.33 KB, patch)
2011-12-01 06:49 PST, Kristóf Kosztyó
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description WebKit Review Bot 2011-07-01 13:47:13 PDT
tests_run0.txt gets clobbered when re-running failing tests
Requested by abarth on #webkit.
Comment 1 Dirk Pranke 2011-07-01 14:35:12 PDT
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.
Comment 2 Eric Seidel (no email) 2011-07-05 13:23:57 PDT
I'm fine with adding a "retries" sub-directory which is used for the re-run.
Comment 3 Eric Seidel (no email) 2011-08-08 13:36:41 PDT
This is a polish bug.
Comment 4 Csaba Osztrogonác 2011-08-12 04:21:32 PDT
This fix would be great to help debugging DRT sideeffect bugs revealed by NRWT.

Kristóf, could you pick up this bug?
Comment 5 Kristóf Kosztyó 2011-11-09 07:13:49 PST
Created attachment 114270 [details]
draft patch

Hi,
I'am working on these bug.
Do you have any idea to make it better?
Comment 6 Ojan Vafai 2011-11-10 08:44:21 PST
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 7 Dirk Pranke 2011-11-10 15:47:19 PST
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.
Comment 8 Eric Seidel (no email) 2011-11-10 16:07:37 PST
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.
Comment 9 Dirk Pranke 2011-11-10 16:11:17 PST
(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.
Comment 10 Dirk Pranke 2011-11-10 16:12:31 PST
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.
Comment 11 Eric Seidel (no email) 2011-11-10 16:14:50 PST
It seems a much simpler solution would just be to change the results directory during retry attempts. :)
Comment 12 Dirk Pranke 2011-11-10 16:18:52 PST
(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).
Comment 13 Eric Seidel (no email) 2011-11-10 16:22:31 PST
I'm not sure results_directory is even a Port concern anymore?  Do ports specify separate retry directories?
Comment 14 Dirk Pranke 2011-11-10 16:32:18 PST
(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.
Comment 15 Kristóf Kosztyó 2011-11-14 07:50:38 PST
I see, then I will rewrite it, and make a unit test too
Comment 16 Kristóf Kosztyó 2011-11-18 07:20:47 PST
Created attachment 115804 [details]
proposed fix with unit test
Comment 17 Dirk Pranke 2011-11-21 12:57:57 PST
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.
Comment 18 Kristóf Kosztyó 2011-11-30 07:44:45 PST
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 19 Dirk Pranke 2011-11-30 12:17:56 PST
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.
Comment 20 Kristóf Kosztyó 2011-12-01 06:49:37 PST
Created attachment 117411 [details]
proposed fix

Thanks for your forbearance
Comment 21 Csaba Osztrogonác 2011-12-01 07:46:58 PST
(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
Comment 22 Kristóf Kosztyó 2011-12-01 07:55:36 PST
(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 23 Dirk Pranke 2011-12-01 13:36:03 PST
Comment on attachment 117411 [details]
proposed fix

clearing r, cq flags.