Open Bug 1564300 Opened 3 years ago Updated 2 years ago

Old ESLint errors may be raised after newer pushes on Phabricator

Categories

(Firefox Build System :: Source Code Analysis, defect, P3)

defect

Tracking

(Not tracked)

People

(Reporter: marcosc, Unassigned)

References

Details

Phabricator URL: https://phabricator.services.mozilla.com/D36885

ESlint on review bot was complaining about "Error: Insert · [eslint: prettier/prettier]". However, doing ./mach eslint locally on the affected file is not picking up the issue.

The original line was:

const manifest = ManifestProcessor.process({ ...args, ...aOptions });

I've had to change it to make ESLint happy on the sever.

Could it be that ESLint on the server is a different version than the one I have locally?

I tried updating by:

./mach  eslint -s

But it didn't help.

The priority flag is not set for this bug.
:sylvestre, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(sledru)

Victor, does it ring a bell?

Flags: needinfo?(sledru) → needinfo?(vporof)

Eslint on the server should be the same version as the one mach installs locally. I think Mark can confirm for sure.

Flags: needinfo?(vporof) → needinfo?(standard8)

I couldn't reproduce locally, so I've just checked through the history.

Diff 8 (128420) is the one that failed the lint job.

In this diff, the line was:

const manifest = ManifestProcessor.process({...args, ...aOptions });

With the error TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/dom/manifest/ManifestObtainer.jsm:120:47 | Insert·(prettier/prettier)

Diff 9 (128423) fixed this:

https://phabricator.services.mozilla.com/D36885?vs=128420&id=128423#toc

Diff 8 was a "fix merge screwup", Diff 9 was also titled "fix merge screwup". Diff 10 was "Make eslint happy".

Diff 9 was pushed about 5 minutes after diff 8. The ESLint issue was reported 37 minutes after Diff 8 was submitted.

So this looks like a case of crossed wires.

Sylvestre, maybe analysis tasks should be cancelled/ignored if they're for an older push?

Flags: needinfo?(standard8) → needinfo?(sledru)
Summary: [Automated review] Unfixable ESLint error → ESLint errors may be raised after newer pushes
Summary: ESLint errors may be raised after newer pushes → Old ESLint errors may be raised after newer pushes on Phabricator

We've had this issue before, but aren't the comments grayed out? I'm not sure we have the possibility just yet to do what comment #4 implies.

Yeah, we have that on our todo list https://github.com/mozilla/code-review/issues/18 but this isn't a top priority.

Flags: needinfo?(sledru)
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.