Add doc ui tests #53319
Add doc ui tests #53319
Conversation
rust-highfive
added
the
S-waiting-on-review
kennytm
added
T-rustdoc
| @@ -109,3 +109,5 @@ version.texi | |||
|
|
|||
| no_llvm_build | |||
|
|
|||
| **node_modules/ | |||
| **doc-ui/.lock | |||
There was a problem hiding this comment.
Hm, do these need to be this much of a blanket ignore? Perhaps we can isolate it to some subpath?
(Very much not important if you don't feel like dealing with it).
There was a problem hiding this comment.
Thanks so much for doing this! Like we've discussed, we can refine things like the exact way we test, but we can get a solid start out of this. I have a few questions for the code.
As for getting this to run, we'll need to make sure that infra can ensure we have a machine with nodejs and puppeteer installed (which will grab chromium for us). I think the only CI machine we have that comes with node is one of the Appveyor builders? @rust-lang/infra, can you confirm?
| "'x86_64-apple-darwin')"); | ||
| console.error(" - second argument: browser path (chrome or chromium)"); | ||
| process.exitCode = 1; // exiting | ||
| }*/ |
There was a problem hiding this comment.
Did you want to do anything with this commented-out CLI validation?
There was a problem hiding this comment.
I still wonder if I should provide some kind of "help" to arguments or not. That's it. :)
| } | ||
| }); | ||
|
|
||
| puppeteer.launch({executablePath: browserPath}).then(async browser => { |
There was a problem hiding this comment.
Is there a way to seed the local storage of Puppeteer? The image that you're checking in doesn't show the type definition, which is the thing that started this project. I remember you mentioning that you wanted to be able to click things on the page later on, but i think at least we should have something like "fold type definition" set to false (or even "expand all" set, if that's possible) so we can properly test the things we wanted to test.
There was a problem hiding this comment.
Yes we can totally. It's part of what I want to add next. I assume we can update local storage pretty easily.
|
|
||
| var outPath = "build/" + toolchain + "/stage" + stage; | ||
| execFileSync(outPath + "/bin/rustdoc", | ||
| ["src/test/doc-ui/lib/lib.rs", "-o", outPath + "/doc-ui"], {}, |
There was a problem hiding this comment.
Is it valid to hard-code the path like this? I fear this will bite us in the travis environment; i've had similar issues with how it sets the current-working-directory before.
There was a problem hiding this comment.
That's how I did with rustdoc-js tests and it's working fine for now.
I think every CI machine comes with node since the rustdoc-js tests already depends on node.js. |
About this: to avoid reinstalling full puppeteer every time we run a test on a platform, we can install our own chrome/chromium and use the last argument to provide its path. Then instead of install puppeteer, we'll have to install puppeteer-core (which is very small in size) but versions have to match the browser we picked! |
|
I'm not sure I follow fully, but could we just add it to one or more docker containers? That would be the approach I'd initially suggest, but perhaps the requirements are heavier? |
|
No, it "just" requires a chrome or chromium instance (with a specific version though). If the puppeteer package is already installed, it'd be even simpler I think (like that I could check if puppeteer is installed before starting tests maybe?). |
|
Why can't it be run inside the docker container like we do for Android emulators? |
|
Almost certainly because the Chromium sandbox doesn't work inside Docker, since it uses the same sets of APIs that containers use (namespaces etc): https://github.com/GoogleChrome/puppeteer/blob/master/docs/troubleshooting.md#chrome-headless-fails-due-to-sandbox-issues |
|
I added a check to see if |
|
Ping from triage @QuietMisdreavus: This PR requires your review. |
|
I think the infra team might not have time to look at this until next year due to time limitations. However, if the docs team wants to, I'd not be opposed to landing this now for running tests locally and eventually we'd figure out how to run them in CI, though we'd want to disable them by default in that case and add an explicit warning. |
|
I can setup a server myself if this is the issue, I just need access to one. |
|
Ping from triage @GuillaumeGomez / @rust-lang/infra: How do we move forwards with this PR? |
|
To be discussed I assume. |
XAMPPRocky
added
S-blocked
Centril
added
S-blocked-closed
and removed
S-blocked
|
Triage: there hasn't been any movement here for some time so I'll close the PR for now. |
|
We're still waiting for news from the @rust-lang/infra team. |
|
The infra team responded in #53319 (comment):
This is on our agenda for the new year though, so don't worry, we didn't forget about this! |
|
Per @pietroalbini's note I am closing this to reduce clutter in the interim. |
| found_puppeteer = String::from_utf8_lossy(&output.stdout).contains("puppeteer"); | ||
| } | ||
| } | ||
| if !found_puppeteer { |
There was a problem hiding this comment.
I think the condition is inverted here, you're running the script only if puppeteer was not found?
There was a problem hiding this comment.
Yes it is inverted, good find!
|
Of possible interest - https://github.com/BBC-News/wraith |
Crates.io makes use of https://percy.io/ for similar benefit |
jyn514
added
S-blocked
Before going any further, we need to make a bit of setup of CIs first. I think we'll need @Mark-Simulacrum for this part. :)
Basically, I added the possibility to provide your own chrome/chromium instead of using the one provided by puppeteer. But I still need to add the option into bootstrap, just not sure yet how we want to do it.
Anyway, with this PR, we have the very base of future rustdoc UI tests.
Oh also: should I put images into another repository or not? We talked about it but didn't conclude anything iirc.
r? @QuietMisdreavus