-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Add testdriver support for generate_test_report WebDriver command. #15305
Add testdriver support for generate_test_report WebDriver command. #15305
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestDriver additions now require an RFC in https://github.com/web-platform-tests/rfcs/ to allow fedback from across the project.
In this case it's unclear what specifies the underlying WebDriver API since it isn't part of the WebDriver spec. So your RFC should link to the relevant spec text.
I have opened an RFC issue here: web-platform-tests/rfcs#7 |
Also adds Reporting API tests which utilize this command.
4f3fb41
to
7ce82f8
Compare
RFC has passed |
@jgraham are you satisfied with this change now? |
I think the testdriver changes are OK, but I shouldn't review the actual tests :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, thanks Paul.
The failing Taskcluster run on this PR is because testdriver.js was modified and tests including it were run. If they weren't stable, well then the job fails. Here are the Chrome failures:
And Firefox:
@paulmeyer90 if none of these are due to changes that you made, which it doesn't seem like, then just let me know when this is ready to merge. I'll file bugs about the remaining failures and admin merge this. Sorry about the inconveniece, this is tracked by #14564 but I don't have a good fix for it. |
@foolip None of those appear to be related to my change, since all the tests added by my change pass, and none of those failing tests use the new API that I added (and those are the only changes in this PR). |
Force-merged as per the comment above. |
@Hexcles can you file the bugs as well? |
Also adds Reporting API tests which utilize this command.