code-review icon indicating copy to clipboard operation
code-review copied to clipboard

Test manifest warnings don't appear on phabricator output

Open Standard8 opened this issue 8 months ago • 2 comments

This phabricator revision didn't indicate that there were any warnings from the linters.

However, looking at the logs from the treeherder run, there were warnings given. In this case it was:

TEST-UNEXPECTED-WARNING | /builds/worker/checkouts/gecko/toolkit/mozapps/extensions/test/xpcshell/xpcshell.toml:0 | The manifest sections are not in alphabetical order. (test-manifest-toml)

I've seen in the past warnings from the ESLint runs, so I'd have expected this to report an issue as well.

Standard8 avatar Mar 14 '25 10:03 Standard8

This is not the only instance of missing linting reports; I fixed up many issues that should have been caught by the linter, at https://bugzilla.mozilla.org/show_bug.cgi?id=1953004

Rob--W avatar Mar 14 '25 11:03 Rob--W

I have rebased the patch to https://bugzilla.mozilla.org/show_bug.cgi?id=1953004 a couple of times, and every time a new instance of a failing linting check was introduced. Without enforcement of the check, this is bound to happen more often.

Rob--W avatar Apr 14 '25 08:04 Rob--W

More information from Standard8: Warnings aren't being show in code review bot results.

Test Phabricator revision: https://phabricator.services.mozilla.com/D265816 There's a warning on the line I touched (and several others in the file). The warning on line 17 where the error is, is new. The other ones in the file were pre-existing.

Archaeopteryx avatar Sep 25 '25 14:09 Archaeopteryx

This is another revision, where I accidentally introduced a warning but thankfully my reviewer picked it up.

The warning was reported in the Treeherder log but not reported to Phabricator.

Standard8 avatar Sep 30 '25 20:09 Standard8

I looked into this issue and found that we get a publishable=False response from the backend for these warnings.

As the backend only check if they are at error level OR in patch, it could be either:

  • invalid in_patch state reported from the bot onto the backend (too early, before having the real state of in patch ?)
  • erroneous state for full file issues (on line 0). But this used to work too

I'm trying to find when this behaviour changed

La0 avatar Oct 08 '25 09:10 La0

@La0 the warning mentioned in https://github.com/mozilla/code-review/issues/2665#issuecomment-3334296583 was not a full file one:

{"linter": "eslint", "path": "/builds/worker/checkouts/gecko/toolkit/actors/NetErrorParent.sys.mjs", "lineno": 17, "column": 10, "message": "> resource:///modules/GenAI.sys.mjs is part of Desktop Firefox and cannot be unconditionally used by this code (which has to also work elsewhere).", "hint": null, "source": null, "level": "warning", "rule": "mozilla/no-browser-refs-in-toolkit", "lineoffset": null, "diff": null, "relpath": "toolkit/actors/NetErrorParent.sys.mjs"}

marco-c avatar Oct 08 '25 10:10 marco-c

@La0 the warning mentioned in #2665 (comment) was not a full file one:

Yes, I think it's a general issue (not only tied to full-file)

La0 avatar Oct 08 '25 12:10 La0

This has been resolved.

Archaeopteryx avatar Oct 16 '25 09:10 Archaeopteryx