Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a lint for assert_throws and promise_rejects #21294

Merged
merged 3 commits into from Feb 13, 2020
Merged

Add a lint for assert_throws and promise_rejects #21294

merged 3 commits into from Feb 13, 2020

Conversation

jgraham
Copy link
Contributor

@jgraham jgraham commented Jan 21, 2020

This adds a rather large number of existing uses to the allow list

@jgraham jgraham mentioned this pull request Jan 21, 2020
Merged
zcorpan
zcorpan reviewed Jan 21, 2020
View reviewed changes
lint.whitelist Outdated Show resolved Hide resolved
lint.whitelist Outdated Show resolved Hide resolved
stephenmcgruer
stephenmcgruer approved these changes Jan 21, 2020
View reviewed changes
Copy link
Contributor

@stephenmcgruer stephenmcgruer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some nits (please address before landing)

docs/writing-tests/testharness-api.md Outdated Show resolved Hide resolved
docs/writing-tests/testharness-api.md Outdated Show resolved Hide resolved
docs/writing-tests/testharness-api.md Outdated Show resolved Hide resolved
docs/writing-tests/testharness-api.md Outdated Show resolved Hide resolved
docs/writing-tests/testharness-api.md Outdated Show resolved Hide resolved
docs/writing-tests/testharness-api.md Outdated Show resolved Hide resolved
docs/writing-tests/testharness-api.md Outdated Show resolved Hide resolved
tools/lint/rules.py Outdated Show resolved Hide resolved
@stephenmcgruer stephenmcgruer mentioned this pull request Jan 21, 2020
Closed
@gsnedders
Copy link
Member

This adds a rather large number of existing uses to the allow list

What is the performance impact of this? Historically adding large numbers of items here has been problematic, given we scan through the entire list each time.

@jgraham
Copy link
Contributor Author

jgraham commented Jan 21, 2020

What is the performance impact of this?

I'm not sure although since @bzbarsky is actively working through the backlog, I'm also not that worried. I agree that the code here highly suboptimal…

@bzbarsky
Copy link
Contributor

Basically, once #21286 merges I'll be able to put up my pr to get rid of most of the callsites for assert_throws. Once that merges I'll poke at promise_rejects.

@bzbarsky bzbarsky mentioned this pull request Jan 22, 2020
Closed
@gsnedders gsnedders closed this Jan 24, 2020
@gsnedders gsnedders deleted the lint_assert_promise branch January 24, 2020 18:07
@gsnedders gsnedders restored the lint_assert_promise branch January 24, 2020 18:45
@Hexcles Hexcles reopened this Jan 24, 2020
@stephenmcgruer stephenmcgruer mentioned this pull request Jan 25, 2020
Closed
@stephenmcgruer
Copy link
Contributor

@jgraham With Boris' many changes over the last few days, I think the files added to the lint can be pared down significantly here now! :)

@gsnedders gsnedders removed their assignment Jan 31, 2020
@jgraham
Copy link
Contributor Author

jgraham commented Feb 5, 2020

So FWIW I measured the perf impact of this and we're so comically slow parsing HTML that nothing else makes a meaningful difference. With an updated list with bz's changes for assert_throws we spend < 2% of the runtime checking for allowed failures

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-21294 February 5, 2020 11:09 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-21294 February 5, 2020 12:00 Inactive
stephenmcgruer
stephenmcgruer approved these changes Feb 5, 2020
View reviewed changes
Copy link
Contributor

@stephenmcgruer stephenmcgruer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, though Boris may have changed your whitelist yet again with their latest promise_rejects PR ;)

@jgraham jgraham merged commit 1ebba6d into master Feb 13, 2020
@jgraham jgraham deleted the lint_assert_promise branch February 13, 2020 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zcorpan zcorpan zcorpan left review comments

@domenic domenic domenic left review comments

@stephenmcgruer stephenmcgruer stephenmcgruer approved these changes

@gsnedders gsnedders Awaiting requested review from gsnedders

@jugglinmike jugglinmike Awaiting requested review from jugglinmike

@sideshowbarker sideshowbarker Awaiting requested review from sideshowbarker

@Hexcles Hexcles Awaiting requested review from Hexcles

@KyleJu KyleJu Awaiting requested review from KyleJu

@LukeZielinski LukeZielinski Awaiting requested review from LukeZielinski

Assignees

@domenic domenic

Labels
docs infra lint testharness.js
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@jgraham @gsnedders @bzbarsky @stephenmcgruer @zcorpan @domenic @Hexcles @wpt-pr-bot @moz-wptsync-bot