-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
[WIP] Add rustdoc GUI tests #70533
[WIP] Add rustdoc GUI tests #70533
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.
Sorry for the delay here, have been quite busy.
90d7118
to
6e9729e
Compare
Updated: I moved what was needed into bootstrap (so the cloning, the setup and the run). Only kept the required dependencies into the docker file. |
This comment has been minimized.
This comment has been minimized.
6e9729e
to
ef95da7
Compare
This comment has been minimized.
This comment has been minimized.
Applied the comments, sorry for the long delay. |
b827068
to
ea7d406
Compare
libexpat1-dev \ | ||
gnupg \ | ||
apt-utils \ | ||
wget |
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.
Can we just use curl? Looks like the apparent use is for the wget below but that should be fine to just use curl for I imagine?
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 guess, I tend to prefer wget because it's simpler but we can use curl.
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.
However, I just copied the code from the chromium repository so I'd prefer not to change it until everything is working so we can use the current code as witness.
ea7d406
to
ade95aa
Compare
Updated. |
Okay, so this is looking relatively good to me -- I guess maybe we should try to test it? We'll want to add a dedicated Makefile target I think, like check-aux but with GUI tests enabled too (perhaps check-aux-and-gui or something). That can be done roughly here: rust/src/bootstrap/mk/Makefile.in Lines 43 to 47 in a8cf399
By adding something like:
We'll also want to enable this builder on try branch, by applying this diff: diff --git a/src/ci/azure-pipelines/try.yml b/src/ci/azure-pipelines/try.yml
index 818306a0092..eea08d1f26a 100644
--- a/src/ci/azure-pipelines/try.yml
+++ b/src/ci/azure-pipelines/try.yml
@@ -26,6 +26,7 @@ jobs:
strategy:
matrix:
dist-x86_64-linux: {}
+ x86_64-gnu-aux: {}
# The macOS and Windows builds here are currently disabled due to them not being
# overly necessary on `try` builds. We also don't actually have anything that Once that's done we can run bors try here on this PR and see what the results are. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
💔 Test failed - checks-actions |
So at this point, only 2 tests are failing, however I need to be able to get the images to understand why. More globally, we need to provide an easy way for contributors to get the images to see what went wrong. cc @rust-lang/infra |
I still am uncertain what exactly you mean by images. Is that the docker images? A model like the one I proposed here might be necessary: rust-lang/triagebot#760 |
Images like PNGs. https://github.com/GuillaumeGomez/test-rust-docs-ui/tree/master/ui-tests |
The images generated in case of failures (the PNG screenshots). |
Yeah, then I think the triagebot issue I linked to is the best bet for the most up to date mentor-ish issue on integrating retrieval of artifacts into CI. I would comment there to sync up with @lcnr though who expressed interest in working on this. |
Fine by me. In the meantime, I'd be really interested into being able to know where the images are (no need for automation on this part I hope) so I can at least move forward on this PR. :) |
I am confused - you believe you have uploaded them? We don't upload by default from non dist builders at all... |
Oh snap... Any idea on how we could get access to the images generated then? |
...I have already pointed you at an issue typed up with docs on how I would suggest going about doing this? If something in that description is unclear, please comment on that issue with questions. It doesn't require triagebot integration, just work in CI here, for basically all of the steps. |
I misundestood the issue then... Going to re-read it. |
☔ The latest upstream changes (presumably #77294) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
I'm going to close this for now to keep the list of PRs assigned to me cleaner, but feel free to reopen when you have more time to work on it! |
…ark-Simulacrum Rustdoc gui tests This is a reopening of rust-lang#70533. For this first version, there will be no screenshot comparison. Also, a big change compared to the previous version: the tests are now hosted in the rust repository directly. Since there is no image, it's pretty lightweight to say the least. So now, only remains the nodejs script to run the tests and the tests themselves. Just one thing is missing: where should I put the documentation for these tests? I'm not sure where would be the best place for that. The doc will contain important information like the documentation of the framework used and how to install it (`npm install browser-ui-test`, but still needs to be put somewhere so no one is lost). We'd also need to install the package when running the CI too. For now, it runs as long as we have nodejs installed, but I think we don't it to run in all nodejs targets? cc `@jyn514` r? `@Mark-Simulacrum`
[TODO]