Rustdoc gui tests

[GuillaumeGomez]

This is a reopening of #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

[GuillaumeGomez]

Also: weird CI failure...

[jyn514]

where should I put the documentation for these tests?

I think either a README in the directory they're in, or in the dev-guide would be a good place. Possibly one could link to the other.

[Mark-Simulacrum]

I am still a little uncertain about the approach here of needing a JS out-of-tree dependency expected to be installed through nodejs/npm; I definitely think this cannot land without some amount of published documentation on how to update these tests. I am a bit worried that it's going to be a significant increase in the overhead for contributors to rustdoc (needing a JS toolchain installed to test things locally), but I could be wrong about that. Ultimately that's a T-rustdoc decision, I suppose.

The comment I left should hopefully fix CI.

[GuillaumeGomez]

We can clone the framework too. I don't have much preference for a solution, just thought that it might be simpler to use npm to do so. In the previous version, it's true that we cloned the repository inside bootstrap script and then run it. So whatever way you prefer.

[Mark-Simulacrum]

Even cloning the framework presumably doesn't decouple us from needing the JS toolchain though? I think that's my primary concern. If it's just nodejs that's not too bad, but I'm a bit worried about running npm install mid-build.

[GuillaumeGomez]

Then let's go for a submodule. Like that at least we'll know what's going on and no need for npm.

[GuillaumeGomez]

To make things a bit simpler to track down, I made the browser-UI-test into a submodule instead of relying completely on npm. However, we still need to install puppeteer and pngjs with npm. Another concern that @Mark-Simulacrum had was that the test suite would be run too often, so to prevent this to happen, I added the following checks:

• You need both npm and nodejs to be installed
• You need the browser-UI-test dependencies to be installed (through npm) as well

In case only the second condition isn't respected, a warning is displayed.

The last commit is the CI update. I took the changes mostly from #70533 but we'll still need to have @Mark-Simulacrum to confirm them. :)

So once everything else has been validated, I'll add the documentation.

[Mark-Simulacrum]

If we're going to require npm anyway I see no reason for the submodule I think? We probably want to put the lockfile in git though...

[GuillaumeGomez]

That's the thing: either we put a lockfile in rust repository, or we keep the submodule and use its own lockfile. One way or another, it's fine by me. Maybe you have an opinion @jyn514 ?

[Mark-Simulacrum]

I have several requirements:

• we should use published (to npm or crates.io etc) deps or rust-lang repositories in submodules, to help ensure they don't go away on us; regardless a lockfile of some kind should exist.
• it should be easy to ensure that the test here is run by CI (e.g. minimal checking for just node is fine); if we start checking for the presence of multiple binaries I'd rather just gate on a new config.toml option

It's up to the rustdoc team whether you think this requirement is a reasonable ask of contributors - it seems like a high bar to me, but maybe editing these tests is rare and not too worrisome.

I will take a look at this PR's implementation soon.

[GuillaumeGomez]

These tests are mostly for me. To be more precise: I spend most of my time fixing GUI issues and I'd that to slow down quite a lot. So unless someone is changing the front-end, they don't need to run this test suite, and even if they do, I'm pretty sure in most cases there won't be an issue (but if there is one, the CI will tell us, finally!).

[jyn514]

@GuillaumeGomez that's not how testing works, though. CI can fail on anyone's changes, not just yours. The scenario is someone does make changes and either needs to add a new test or an existing test fails.

That said, I think if we print "run npm install x" if the package isn't installed, that's good enough - I'd expect anyone modifying the frontend to at least be familiar with npm even if it's not currently installed.

[GuillaumeGomez]

I maybe badly tried to explain my point of view. The idea was "if you broke the GUI tests, you very likely know how npm works". 😄

[GuillaumeGomez]

@Mark-Simulacrum I might need help for the CI scripts update (not too sure what to change here without breaking everything...). So whenever you have time, it'd be super appreciated! :)

[Mark-Simulacrum]

I think this is still adding a submodule which seems like the wrong approach. I would expect us to use browser-ui-test just like we expect e.g. a C compiler to be installed to run this test. I presume that it can be readily installed by contributors via npm or so?

[GuillaumeGomez]

Hmmm... Well, it doesn't hurt to have package.json and package-lock.json files instead of a submodule I guess. I'll make the update soon. But still need your help for the CI files though. ;)

[GuillaumeGomez]

Commits squashed, src/ci/github-actions/ci.yml changes were removed as well. Thanks a lot for your help!

@bors: r=Mark-Simulacrum

[bors]

📌 Commit 20f2497 has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit 20f2497 with merge f8297b6ad3ae48ff88b8cdd31dcc8fbe34355050...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

A job failed! Check out the build log: (web)(plain)

Click to see the possible cause of the failure (guessed by this bot)

[GuillaumeGomez]

There is no failure apparently? Well, let's rety...

@bors: retry

[bors]

⌛ Testing commit 20f2497 with merge 9d32bf0a81931e6e833eb0155aa5f067e7b7b3f6...

[bors]

💥 Test timed out

[rust-log-analyzer]

A job failed! Check out the build log: (web)(plain)

Click to see the possible cause of the failure (guessed by this bot)

[ehuss]

@bors retry
macos: system is shutting down

[bors]

⌛ Testing commit 20f2497 with merge 352238d...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 352238d to master...

[jethrogb]

I don't think this is valid syntax. At least I can't find it in man gitignore and ripgrep complains about it:

rust/.gitignore: line 55: error parsing glob '**node_modules': invalid use of **; must be one path component
rust/.gitignore: line 56: error parsing glob '**package-lock.json': invalid use of **; must be one path component

[GuillaumeGomez]

** is to look for all subfolders, whatever the level.

[jethrogb]

I believe that would be **/node_modules

[GuillaumeGomez]

It works for me as is but don't hesitate to send an update.