Bug 103839 – FlakyTestReporter should be re-enabled and taught how to post patches

• Component: New Bugs
• Status: REOPENED

[Eric Seidel (no email)]

FlakyTestReporter should be re-enabled and taught how to post patches

The FlakyTestReporter used to file nag-bugs, documenting tests that we saw flake on the EWS/commit-queue bots.

We disabled it because the nag-bugs weren't really that helpful.

However, now we could re-enable it and have it file bugs with patches to fix test-expectations to document the flake!

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/bot/flakytestreporter.py

Flaky tests are a big problem for the EWS/CQ bots.  The bots are smart enough to recover from occasionally flaky tests (up to 10 at a time), but:
1.  can't handle lots of flaky tests at once (there is a hard-coded limit before the bot gives up and assumes the tree is too unstable to determined anything about the patch)
2.  can't handle tests which flake super-often (the bot re-runs the tests to try and get repeatable results, very-flaky tests can trick the bot into thinking they're caused by the patch under testing and cause false-rejections).

The FlakyTestReporter tried solved these problems by nagging humans, but it could solve it more directly by posting a patch for r?/cq? with the needed lines for TestExpectations.

[Eric Seidel (no email)]

Here are some examples of the bugs the FlakyTestReporter used to file:
bug 80012, bug 79894, bug 79845, bug 79842

[Eric Seidel (no email)]

For those who are curious how/why the bot has a hard-coded limit about the number of flakes/failures it can handle while still processing patches, see:
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/bot/expectedfailures.py#L40
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/bot/layouttestresultsreader.py#L68
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/steps/runtests.py#L38
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/net/layouttestresults.py#L152

(I was wrong, the current limit is 30.)

The gist is that we don't want to run all the tests when a patch makes every test crash (that could take hours) so we stop after some number of failures.  If we ever hit that limit, we can no longer trust the results.

Thus if there are 30 failures/flakes then we can't process patches at all.  We just spin until someone fixes the tree.

The proposal in this bug would help keep the number of flakes down, by making it easy for a human (the current webkit sherriff?) to take action to fix the TestExpectations to recognize the flakes.

[Eric Seidel (no email)]

This may need to bump up in my priority queue.  The EWS is much less useful at current given how many false-rejections it's having due to flakes. :(

[Eric Seidel (no email)]

I believe what's actually going on is that LayoutTestResultsReader doesn't know how to handle NRWT results directories. :)

Thus we're silently failing to parse the results.  Which may mean the whole flakiness avoidance system on the EWS is failing!

[Eric Seidel (no email)]

(In reply to comment #4)
> I believe what's actually going on is that LayoutTestResultsReader doesn't know how to handle NRWT results directories. :)
> 
> Thus we're silently failing to parse the results.  Which may mean the whole flakiness avoidance system on the EWS is failing!

Yes, it turns out this is caused by use of DeprecatedPort, which reports the result directory as /tmp/layout-test-results... which no longer exists.  So we silently fail to archive the results, thus we don't upload them, and we don't know how to check for flaky tests w/o them!

[Eric Seidel (no email)]

(In reply to comment #5)
> (In reply to comment #4)
> > I believe what's actually going on is that LayoutTestResultsReader doesn't know how to handle NRWT results directories. :)
> > 
> > Thus we're silently failing to parse the results.  Which may mean the whole flakiness avoidance system on the EWS is failing!
> 
> Yes, it turns out this is caused by use of DeprecatedPort, which reports the result directory as /tmp/layout-test-results... which no longer exists.  So we silently fail to archive the results, thus we don't upload them, and we don't know how to check for flaky tests w/o them!

I've since learned that non-interactive mode causes webkitpy to pass --results-directory=/tmp/layout-test-results to NRWT when it runs it, so it's possible that this is a red herring.

I've written a patch to fix this all the same. :)

[Eric Seidel (no email)]

Created attachment 181278 [details]
Patch

[Eric Seidel (no email)]

I think the largest obstacle standing in the way of completely removing ports.py, is figuring out how Host is going to hold a Port object (if it should at all).  Right now Port holds a Host, so for Host to hold/create Port could be a circular dependency.

Cleaning up/moving the Port infrastructure into common and clarifying its relationship with Host should make it trivial to just fold the last pieces of ports.py into the modern infrastructure and get rid of other ORWT-dependent bugs like this one.

[Adam Barth]

Comment on attachment 181278 [details]
Patch

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

This looks great!

> Tools/ChangeLog:9
> +        It also adds a real Port object to the EWS and CommitQueue objects

woah

> Tools/Scripts/webkitpy/tool/commands/queues.py:250
> +        return os.path.join(self._log_directory(), "%s.log" % patch.bug_id())

Not self._tool.filesystem.join ?

> Tools/Scripts/webkitpy/tool/commands/queues.py:273
> +        if not self.port_name:
> +            return

Should this assert instead?

[Eric Seidel (no email)]

(In reply to comment #9)
> (From update of attachment 181278 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181278&action=review
> 
> This looks great!
> 
> > Tools/ChangeLog:9
> > +        It also adds a real Port object to the EWS and CommitQueue objects
> 
> woah

Yeah.  It's a layering violation until we move Port down into common, sadly. :(

> > Tools/Scripts/webkitpy/tool/commands/queues.py:250
> > +        return os.path.join(self._log_directory(), "%s.log" % patch.bug_id())
> 
> Not self._tool.filesystem.join ?

Old code.  Will fix.

> > Tools/Scripts/webkitpy/tool/commands/queues.py:273
> > +        if not self.port_name:
> > +            return
> 
> Should this assert instead?

This is part of why PatchProcessingQueue is kinda lame.  It was intended for only the EWS and CQ, but its also used by the StyleQueue... the StyleQueue (currently) has no port_name.

[Eric Seidel (no email)]

Comment on attachment 181278 [details]
Patch

Eh.  Decided it was not worth fixing the old os.path usage (it's possible it's called before _tool is set!).  Will get that in another round.

[WebKit Review Bot]

Comment on attachment 181278 [details]
Patch

Clearing flags on attachment: 181278

Committed r138826: <http://trac.webkit.org/changeset/138826>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

It appears this neither fix nor broke things.  I guess I'll have to take a second crack at it.

[Dirk Pranke]

Comment on attachment 181278 [details]
Patch

Just some semi-random drive-by comments as I get caught up on my bug backlog ...

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

>>> Tools/ChangeLog:9
>>> +        It also adds a real Port object to the EWS and CommitQueue objects
>> 
>> woah
> 
> Yeah.  It's a layering violation until we move Port down into common, sadly. :(

Why do you think this is a layering violation? Do you think any code shared by multiple packages should be in webkitpy.common? 

(That seems unnecessary to me; I wouldn't have a "common" directory at all, since such things tend to grow arbitrarily, as ours has, and just push things down a level, e.g. we should really have webkitpy.scm and webkitpy.checkout).

> Tools/Scripts/webkitpy/tool/commands/queues.py:259
> +    def _new_port_name_from_old(self, port_name):

feels like this should be in PortFactory() instead, maybe? it doesn't seem like it's specific to EWS or the PatchProcessingQueue.

> Tools/Scripts/webkitpy/tool/commands/queues.py:263
> +        # ApplePort.determine_full_port_name asserts if the name doesn't include version.

Not exactly - 'mac' asserts if you're not running on a mac, since we can't determine which version to use by default. on a mac, it picks the host machine's version.

>>> Tools/Scripts/webkitpy/tool/commands/queues.py:273
>>> +            return
>> 
>> Should this assert instead?
> 
> This is part of why PatchProcessingQueue is kinda lame.  It was intended for only the EWS and CQ, but its also used by the StyleQueue... the StyleQueue (currently) has no port_name.

This seems a bit confusing ... the style queue is probably happy using the "default" port for the platform it is running on (since any port should do). So maybe you should always instantiate a self._port from the port factory?

> Tools/Scripts/webkitpy/tool/commands/queues.py:275
> +        self._deprecated_port = DeprecatedPort.port(self.port_name)

maybe you should just store the flag here, and not keep the deprecated port around, in order to prevent others from using it?