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.

Sign up for GitHub

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 console.countReset manual test #10722

Merged
merged 3 commits into from
May 9, 2018
Merged

Add console.countReset manual test #10722

merged 3 commits into from
May 9, 2018

Conversation

domfarolino
Copy link
Member

@domfarolino domfarolino commented Apr 30, 2018 •
edited by wpt-pr-bot
Loading

Tests for the pending Console Standard PR specifying console.countReset. This set of tests reflects the current state of that PR, which is subject to change, depending on implementer agreement. Do not merge yet.

@wpt-pr-bot wpt-pr-bot requested review from domenic and pwnall April 30, 2018 21:15
@domfarolino domfarolino mentioned this pull request May 4, 2018
Merged
pwnall
pwnall approved these changes May 4, 2018
View reviewed changes
Copy link
Contributor

@pwnall pwnall 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 one nit, thank you for the new test!

console/console-countReset-logging-manual.html
@@ -0,0 +1,47 @@
<!DOCTYPE html>
<html>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: You don't need <html> <head> or <body>.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, right! The reason all of the manual tests under console/ have these unneeded components is because when I added the first one a while ago I just copied the format of some other manual tests in this repo. If you'd like I can remove these extra bits from this test (+ other manuals in this folder)?

@Ms2ger Ms2ger changed the title Add consoleReset manual test Add console.countReset manual test May 9, 2018
domenic
domenic approved these changes May 9, 2018
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM. Also worth adding some non-manual tests for the string conversion.

@domfarolino
Copy link
Member Author

Done! Since we're basically testing IDL conversions for a lot of console methods at this point, I added a TODO for me to DRY this up a bit later. We could probably move all these to a console-label-conversions.any.js or something of the sort.

@domenic
Copy link
Member

domenic commented May 9, 2018

Yeah, I imagine idlharness might even work, if it's gotten namespace support recently.

@domfarolino
Copy link
Member Author

Right now that's on my backlog to look at: #7583

@domfarolino domfarolino merged commit bd9f440 into web-platform-tests:master May 9, 2018
@domfarolino domfarolino deleted the console-reset-manual branch May 9, 2018 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domenic domenic domenic approved these changes

@pwnall pwnall pwnall approved these changes

Assignees
No one assigned
Labels
console
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@domfarolino @domenic @pwnall @wpt-pr-bot