Gecko exported tests sometimes contain lint errors

[Hexcles]

Gecko export #8261 introduced a few tests that have lint errors, especially the multiple uses of setTimeout.

This is currently blocking chromium import.

By the way, @jgraham wpt lint should have been run against the change somewhere in the process, either before committing to mozilla-central, or before merging (Chromium actually does both). And for this change, it seems neither happened. Is this a bug?

[Hexcles]

cc @rakuco , who's on ecosystem-infra rotation.

[jdm]

The lint does run in Gecko as far as I know; the problem is that the upstream process happens in batches, so newer lints that have not reached mozilla-central can miss tests that were written in the interval.

[rakuco]

Thanks for the fix, I can confirm lint has stopped blocking our imports.

[Hexcles]

Thank you, @jdm.

So supposedly the timeline is:

1. The change was submitted to Gecko and passed the lint Gecko had at that time (which didn't have rules for setTimeout, testRunner, etc.).
2. New rules were introduced to lint.
3. The changes were exported to GitHub, and failed the new lint rules.

If we don't want to change the current Gecko export workflow too much, one thing we can do for sure is to wait for Travis CI before merging the export PRs. WDYT, @jgraham ?

[Hexcles]

The immediate problem has been fixed by #8272.

As @jgraham points out on IRC, Travis checks are currently skipped during Gecko exports because they are too slow for the current Gecko export workflow. (A bunch of commits are exported one time, and each has a PR, which would create a long queue on Travis.) Besides, we can always miss things because of racing conditions regardless: new lints can be introduced between a PR passes Travis and lands in master.

[foolip]

I'm fixing more cases in #8294, those were in Python code. @andreastt FYI.

[foolip]

@jgraham, what would it take to make this much less likely to happen? Is it blocked on the new Gecko 2-way sync?

[andreastt]

Is it the slowness of wpt lint that prevents us from hooking it up to mozlint? AIUI m-c’s default linter runs on touched files, so if it supported that maybe not every file had to be linted every time.

flake8, using the configuration already available in WPT, seems easier to hook up becuase there is already plumbing in mozlint for that.

[jgraham]

wpt lint is hooked up. Since the rules can change it doesn't always catch everything. The python lint isn't hooked up, but I think we are already running flake8 for some other things, so that's probably possible to set up; I don't know if we can ensure we are using the right rules.

[Hexcles]

Yeah, the underlying root cause is a race condition that's hard to be fixed: changes can be made on the vendor side while new lint rules are being added to this GitHub repo. (There's a more extreme scenario in my comment above, but that's quite rare and is not the direct cause of the issue we are discussing.)

However, one easy way to mitigate the problem is to sync more often, which alleviate this race condition in two folds:

1. Frequent GitHub -> m-c sync ensures m-c has up-to-date lint rules most of the time.
2. If m-c -> GitHub sync happens more often (say right after each commit lands), then we'll be able to wait for the Travis checks to complete before merging the PR, which is not quite feasible in the current batch/burst mode.

Presumably Mozilla's new two-way sync will improve on this very aspect?

[foolip]

Was there really a race in this case? Sounds like there's just some missing lint that's been missing for some time?

[Hexcles]

@foolip Well, from what we've heard from moz folks, m-c is running wpt lint (there might be some discrepancies in Python files, but lints for tests should be the same). When the changes in question were committed, they passed all the lints in m-c at the time. Either the lints in m-c at the time were already out-of-date, or new lints were added between the changes landed in m-c and exported to GitHub. Both would lead to this fail mode. And they are both essentially race conditions.

[foolip]

Right, some race conditions are inevitable, but if most of the cases could be avoided by just being more in sync, then I wouldn't be very concerned about the fact that a race remains.

[foolip]

To clarify: it's inevitable with 2-way sync because we don't have cross-repo atomic commits, so however hard we try we can get into situations that require a human to resolve. Until superhuman AI.

[gsnedders]

My hypothesis is that wpt lint has been asserting in mozilla-central for a long time and hence not running to completion, and nobody noticed it was failing with an uncaught exception.

[foolip]

https://bugzilla.mozilla.org/show_bug.cgi?id=1428582 is now resolved.

@gsnedders, do you think that this issue itself is now then also solved?

[gsnedders]

I would assume so.