Closed Bug 1392787 Opened 6 years ago Closed 6 years ago

Prevent the use of comments to disable tests in manifestparser .ini manifests

Categories

(Testing :: General, enhancement)

Product:
Testing
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(3 files)

Bug 1392787 - [mozlint] Fix bugs in path filtering and add a test,
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1392787 - Add a linter to prevent comment-disabling tests in manifestparser manifests,
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1392787 - Disable manifestparser tests using 'disabled' key instead of comment,
59 bytes, text/x-review-board-request
jmaher
: review+
Details 
There are a bunch of test manifests that disable tests using comments:
http://searchfox.org/mozilla-central/search?q=%5E%5Cs*%28%23%7C%3B%29%5Cs*%5C%5B&case=true&regexp=true&path=ini%24

These should all use the disable=reason syntax instead so that they can be found by automated tools. We should fix the existing occurrences and add a linter to prevent future occurrences.
Comment on attachment 8900278 [details]
Bug 1392787 - [mozlint] Fix bugs in path filtering and add a test,

https://reviewboard.mozilla.org/r/171668/#review176824

the test cases seem to provide decent coverage, the pathutils.py is more of a rubber stamp.
Attachment #8900278 - Flags: review?(jmaher) → review+
Comment on attachment 8900279 [details]
Bug 1392787 - Add a linter to prevent comment-disabling tests in manifestparser manifests,

https://reviewboard.mozilla.org/r/171670/#review176828

::: tools/lint/test-disable.yml:3
(Diff revision 1)
> +---
> +no-comment-disable:
> +    description: "Use 'disable=<reason>' to disable a test instead of a comment"

this doesn't enforce the disable=<reason>, it just makes sure the line doesn't start with ; or #.

could we adjust the description slightly?
Attachment #8900279 - Flags: review?(jmaher) → review+
Comment on attachment 8900280 [details]
Bug 1392787 - Disable manifestparser tests using 'disabled' key instead of comment,

https://reviewboard.mozilla.org/r/171672/#review176830

please test this in bulk!!

::: browser/components/customizableui/test/browser.ini:45
(Diff revision 1)
>  [browser_918049_skipintoolbarset_dnd.js]
>  [browser_923857_customize_mode_event_wrapping_during_reset.js]
>  [browser_927717_customize_drag_empty_toolbar.js]
>  
> -# Bug 1163231 - Causes failures on Developer Edition on Windows 7.
> -# [browser_932928_show_notice_when_palette_empty.js]
> +[browser_932928_show_notice_when_palette_empty.js]
> +disabled=Bug 1163231 - Causes failures on Developer Edition on Windows 7.

with this comment we should just:
skip-if = os == "win"

::: devtools/client/framework/test/browser.ini:92
(Diff revision 1)
>  [browser_toolbox_options_disable_cache-02.js]
>  [browser_toolbox_options_disable_js.js]
>  [browser_toolbox_options_enable_serviceworkers_testing.js]
> -# [browser_toolbox_raise.js] # Bug 962258
> -# skip-if = os == "win"
> +[browser_toolbox_raise.js]
> +disabled=Bug 962258
> +skip-if = os == "win"

this looked to be flaky on osx 4 years ago, please test this with 20 retriggers on each config.
Attachment #8900280 - Flags: review?(jmaher) → review+
Comment on attachment 8900279 [details]
Bug 1392787 - Add a linter to prevent comment-disabling tests in manifestparser manifests,

https://reviewboard.mozilla.org/r/171670/#review176828

> this doesn't enforce the disable=<reason>, it just makes sure the line doesn't start with ; or #.
> 
> could we adjust the description slightly?

The description is actually what gets printed when the lint fails, so it'll look something like:
ERROR 1:1 Use 'disable=<reason>' to disable a test instead of a comment (no-comment-disable)

I'm open to other messages, but I think it's important to mention disable=<reason> so that the developer knows how to fix it. Also it should only be one line.
Comment on attachment 8900279 [details]
Bug 1392787 - Add a linter to prevent comment-disabling tests in manifestparser manifests,

https://reviewboard.mozilla.org/r/171670/#review176828

> The description is actually what gets printed when the lint fails, so it'll look something like:
> ERROR 1:1 Use 'disable=<reason>' to disable a test instead of a comment (no-comment-disable)
> 
> I'm open to other messages, but I think it's important to mention disable=<reason> so that the developer knows how to fix it. Also it should only be one line.

ok, I didn't realize this was an 'error' message, that seems ok.
Comment on attachment 8900280 [details]
Bug 1392787 - Disable manifestparser tests using 'disabled' key instead of comment,

https://reviewboard.mozilla.org/r/171672/#review176830

> with this comment we should just:
> skip-if = os == "win"

This also fails on linux, I'll update the comment.

> this looked to be flaky on osx 4 years ago, please test this with 20 retriggers on each config.

This fails pretty consistently on all platforms.
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf5a6ff97a8f
[mozlint] Fix bugs in path filtering and add a test, r=jmaher
https://hg.mozilla.org/integration/autoland/rev/2ce8a387fa80
Add a linter to prevent comment-disabling tests in manifestparser manifests, r=jmaher
https://hg.mozilla.org/integration/autoland/rev/e774700fe070
Disable manifestparser tests using 'disabled' key instead of comment, r=jmaher
These got backed out for build bustage.
Flags: needinfo?(ahalberstadt)
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6cc7f7a8ebdd
Backed out changeset e774700fe070 for build bustage a=backout CLOSED TREE
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c1fdb1c43538
Backed out 2 changesets for build bustage a=backout CLOSED TREE
The test runs in `make check` on Windows. I had done some extensive testing on try, but didn't realize that artifact builds don't run `make check` so missed the failure. I'll fix it up tomorrow.
Flags: needinfo?(ahalberstadt)
Depends on: 1399522
Sorry took me awhile to look into this. The Windows issue should now be fixed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=97b2ccfa004ccaf7d7ab402ff1d7b20740c22c2a

Joel, not sure if you wanted to look at the interdiff or not, but in case you did here it is:
https://reviewboard.mozilla.org/r/171668/diff/2-3/

(the ancestors stuff is from a rebase)
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee4e36ce2400
[mozlint] Fix bugs in path filtering and add a test, r=jmaher
https://hg.mozilla.org/integration/autoland/rev/68d44e7679e2
Add a linter to prevent comment-disabling tests in manifestparser manifests, r=jmaher
https://hg.mozilla.org/integration/autoland/rev/79e75fbda533
Disable manifestparser tests using 'disabled' key instead of comment, r=jmaher
You need to log in before you can comment on or make changes to this bug.