Bug 63844 - tests_run0.txt gets clobbered when re-running failing tests
: tests_run0.txt gets clobbered when re-running failing tests
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 64491
  Show dependency treegraph
 
Reported: 2011-07-01 13:47 PST by
Modified: 2011-12-01 13:36 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-07-01 13:47:13 PST
tests_run0.txt gets clobbered when re-running failing tests
Requested by abarth on #webkit.
------- Comment #1 From 2011-07-01 14:35:12 PST -------
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 From 2011-07-05 13:23:57 PST -------
I'm fine with adding a "retries" sub-directory which is used for the re-run.
------- Comment #3 From 2011-08-08 13:36:41 PST -------
This is a polish bug.
------- Comment #4 From 2011-08-12 04:21:32 PST -------
This fix would be great to help debugging DRT sideeffect bugs revealed by NRWT.

Kristóf, could you pick up this bug?
------- Comment #5 From 2011-11-09 07:13:49 PST -------
Created an attachment (id=114270) [details]
draft patch

Hi,
I'am working on these bug.
Do you have any idea to make it better?
------- Comment #6 From 2011-11-10 08:44:21 PST -------
(From update of attachment 114270 [details])
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 From 2011-11-10 15:47:19 PST -------
(From update of attachment 114270 [details])
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 From 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 From 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 From 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 From 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 From 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 From 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 From 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 From 2011-11-14 07:50:38 PST -------
I see, then I will rewrite it, and make a unit test too
------- Comment #16 From 2011-11-18 07:20:47 PST -------
Created an attachment (id=115804) [details]
proposed fix with unit test
------- Comment #17 From 2011-11-21 12:57:57 PST -------
(From update of attachment 115804 [details])
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 From 2011-11-30 07:44:45 PST -------
Created an attachment (id=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 From 2011-11-30 12:17:56 PST -------
(From update of attachment 117193 [details])

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 From 2011-12-01 06:49:37 PST -------
Created an attachment (id=117411) [details]
proposed fix

Thanks for your forbearance
------- Comment #21 From 2011-12-01 07:46:58 PST -------
(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
------- Comment #22 From 2011-12-01 07:55:36 PST -------
(In reply to comment #21)
> (In reply to comment #20)
> > Created an attachment (id=117411) [details] [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 From 2011-12-01 13:36:03 PST -------
(From update of attachment 117411 [details])
clearing r, cq flags.