rustdoc-gui test suite not run on CI?

[GuillaumeGomez]

It seems like the rustdoc-gui test suite isn't run on CI because it fails when run locally (as it should) because recent changes modified DOM, making the nojs-attr-pos fail.

cc @Mark-Simulacrum

[jyn514]

@GuillaumeGomez this happens because you never installed node in the CI environment:

Set({"src/test/rustdoc-gui"}) not skipped for "bootstrap::test::RustdocGUI" -- not in ["src/tools/tidy"]
Set({"src/tools/rustdoc-themes"}) not skipped for "bootstrap::test::RustdocTheme" -- not in ["src/tools/tidy"]
No nodejs found, skipping "src/test/rustdoc-gui" tests

This makes me even more certain that both this testsuite and #84480 should be run unconditionally in CI.

[GuillaumeGomez]

But I did install node: https://github.com/rust-lang/rust/pull/79979/files#diff-855904217a51ab471ca6d49c7ecaa231a5a3812d10dc110c4d40dfb8e24bc708R33

[jyn514]

That only runs on x86_64-gnu-aux, which I think isn't run on PRs. I can't find any mentions of it at all except this one:

rust/src/ci/github-actions/ci.yml

Lines 406 to 407 in 06f0adb

	- name: x86_64-gnu-aux
	<<: *job-linux-xl

Which job do you think it runs on?

[GuillaumeGomez]

The one that is run by github actions (one of the 3 "checks" that is run when you open a PR).

[jyn514]

No, none of those jobs are x86_64-gnu-aux. You need to move it to job-linux-xl or x86_64-gnu-llvm-10 if you want it to run on PRs.

[jyn514]

And again, I really think this is the wrong approach and it should be run unconditionally. Otherwise it will break again in the future.

[GuillaumeGomez]

I agree with you. The problem here is that installing puppeteer is quite tricky (because it requires a specific chrome instance). And I have no idea how to handle node/npm install on non-linux targets.

[jyn514]

Ok, can you instead add an assert to bootstrap that makes sure the tests aren't skipped on any linux platform? That should be easier than trying to get this to work on all tier 1 targets.

[Mark-Simulacrum]

Please don't do that, we're explicitly requesting it on one of the builders, so we can simply avoid marking the job as default and change the nodejs conditional to a requirement.

[GuillaumeGomez]

@Mark-Simulacrum Any tip/example on how to do that?

[jyn514]

@GuillaumeGomez toggle this constant:

rust/src/bootstrap/test.rs

Line 772 in 58bdb08

const DEFAULT: bool = true;

and give a hard error here:

rust/src/bootstrap/test.rs

Lines 797 to 800 in 58bdb08

	println!(
	"warning: rustdoc-gui test suite cannot be run because npm `browser-ui-test` \
	dependency is missing",
	);

[jyn514]

Actually it might even be nicer to leave DEFAULT on but add a default_condition that npm is installed - @Mark-Simulacrum how does that sound?

rust/src/bootstrap/test.rs

Lines 775 to 777 in 58bdb08

	fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
	run.path("src/test/rustdoc-gui")
	}

[Mark-Simulacrum]

That seems reasonable too.

[GuillaumeGomez]

Ok, gonna send a PR for it tomorrow then!

[dns2utf8]

Quick question: Is it documented already that node with some version is one of the optional requirements?

[GuillaumeGomez]

I didn't put a requirement for the node version. And it's in the README in src/test/rustdoc-gui. :)

[jyn514]

This was fixed by #84586.