Closed Bug 1756224 Opened 2 years ago Closed 2 years ago

[perfdocs] Reviewbot comments are providing incorrect usage information

Categories

(Testing :: Performance, defect, P2)

defect

Tracking

(firefox-esr91 unaffected, firefox97 unaffected, firefox98 wontfix, firefox99 wontfix, firefox100 fixed)

RESOLVED FIXED
100 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox97 --- unaffected
firefox98 --- wontfix
firefox99 --- wontfix
firefox100 --- fixed

People

(Reporter: sparky, Assigned: mhentges)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files)

The recommended command for the perfdocs linter is currently broken/outdated in multiple areas because, with little to no warning, the linter usage was changed (see bug in regressed by): https://searchfox.org/mozilla-central/search?q=lint&path=perfdocs&case=false&regexp=false

It's unclear why we can't have the default set to */. for perfdocs either, since it always runs with this setting. I see here that the paths should default to cwd as well so it's really unclear what's going on here or if the mozlint documentation is now outdated as well: https://searchfox.org/mozilla-central/source/python/mozlint/mozlint/cli.py#23

The recommended command for the perfdocs linter is currently broken/outdated in multiple areas because, with little to no warning

Hey, sorry about the regression here, I was wondering who all it would impact and I didn't consider the performance crew πŸ˜…

I see here that the paths should default to cwd as well so it's really unclear what's going on here or if the mozlint documentation is now outdated

The docs are outdated, by default it now uses --outgoing --workdir.

It's unclear why we can't have the default set to */. for perfdocs either, since it always runs with this setting

Hmm, this is a bit ambiguous, but I'll answer both interpretations :)

  1. Why can't ./mach lint have a perfdocs-specific default of using */.?

./mach lint doesn't know who is actively calling it (except by reaching around for environment variables, which is brittle). It'd be better for perfdocs to explicitly declare that it wants the full directory to be linted.

  1. Why can't we change perfdocs to, by default, call ./mach lint with */.?

I think that this is the correct solution πŸ‘

Assignee: nobody → mhentges
Status: NEW → ASSIGNED

Updates parser documentation to clarify that, if no paths are
provided, then only those discovered by --outgoing and --workdir
are linted.

Ideally, the perfdocs linter suggestion to --fix issues would take
into account the paths that were provided, rather than printing the
sure-fire "lint and fix all" solution.

Actually, on further consideration, theoretically such a recommendation
should appear at the ./mach lint level, and not be linter-specific. Oh
well, that's a future improvement :)


This patch updates the perfdocs documentation and suggestions to
successfully operate in the same manner that they did before
bug 1753701. One line was added to the run_perfdocs() docstring to
hint towards the "only lint changed files" approach.

Set release status flags based on info from the regressing bug 1753701

Has Regression Range: --- → yes
Pushed by mhentges@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f71993d94b03
Update MozlintParser docs about default `paths` r=ahal
https://hg.mozilla.org/integration/autoland/rev/f73b04feffdd
Update references to perfdocs linter to explicitly set path r=sparky
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 100 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: