-
Notifications
You must be signed in to change notification settings - Fork 370
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
Infrastructure: Add missing test for 6 examples #1424
Conversation
c4d9749
to
c9d2be1
Compare
@spectranaut it would be good to rebase this one to see how the regression test comment job handles this one |
644eaef
to
4be7896
Compare
Regression test coverage:Examples without any regression tests:
Examples missing some regression tests:
Example pages with Keyboard or Attribute table rows that do not have data-test-ids:
SUMMARY:55 example pages found. ERROR - missing tests: Please write missing tests for this report to pass. |
@jongund, any chance you could code review this PR? It would be nice to get the missing tests down to zero :) |
@spectranaut looks like the prettier stuff caused some conflicts |
Valarie, did you add the missing tests or just reformat the existing test files to use "prettier"? |
@jongund I wrote some tests! I'm not sure why I have a failure on the PR for the js linter though. |
@spectranaut there are some in-line comments on the file tab around |
@nschonni yeah I'm a bit baffled by them, there aren't errors for the use of "no strict" in other files, I can't replicate locally... |
They got removed with #1566 |
I have had this problem with 'strict' in the past too, I don't remember how I fixed it. |
756a9b9
to
2be3fcf
Compare
OK, tested it out locally:
I think the checkout for CI is using the merge commit onto main, so that's why it's showing the errors there and not with just the local branch merge |
@nschonni I think we did a little duplicate work, but thanks for look into it :) I rebased and fixed and force pushed and the linter now passes. Looking into the regression test failure now. |
Ok I'm a bit confused by the regression test failures. It's a time out waiting for a link when testing "key enter" on the link apg example -- which is code this PR does not touch. We have seen this error before and guessed it had to do with the travis check environment's resources in some case, or sending to many requests to wikipedia in another case. I tried testing just link over on the bocoup branch (bocoup#43) with some console logs to try to get a better sense of what is happening, and it failed on time and passed the next. Anyway, on the most recent run off all the checks it passes. I'm going to open a backlog issue to track these link related failures in case they come up again. |
Maybe another 429 response code like the wikipedia ones for the tab tests before? |
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.
I was asked (by @spectranaut) to provide a peer review to help move this forward. I've read through all of the changes here and nothing stands out. When I tested locally, I experienced one test failure that was distinct from failures I experienced when running the test from the master branch, and which is only reproducible on my local machine. I don't consider this a blocking issue.
I've followed the review guidelines and have determined that this work satisfies all of the criteria laid out in that document.
(I'd like to acknowledge that I do not have the same level of familiarity with the project as the regular contributors)
test/tests/button_button-idl.js
Outdated
const ex = { | ||
printSelector: '#action', | ||
toggleSelector: '#toggle', | ||
svg: '#example svg', |
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.
For consistency, I believe this should be named svgSelector
?
@rwaldron |
Thanks @rwaldron ! I adopted your feedback, including simplifying one test that remove the failure you got when you run it locally. |
The failing test is due to the flaky link tests currently being fixed in #1600 |
#1378
This PR adds tests for: