Skip to content
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

Closed

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Mar 29, 2020

[TODO]

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 29, 2020
@Dylan-DPC-zz Dylan-DPC-zz changed the title Add rustdoc GUI tests [WIP] Add rustdoc GUI tests Mar 29, 2020
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a 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.

src/ci/docker/x86_64-gnu-aux/Dockerfile Outdated Show resolved Hide resolved
src/ci/docker/x86_64-gnu-aux/Dockerfile Outdated Show resolved Hide resolved
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2020
@GuillaumeGomez GuillaumeGomez force-pushed the add-rustdoc-gui-tests branch from 90d7118 to 6e9729e Compare April 14, 2020 16:09
@GuillaumeGomez
Copy link
Member Author

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.

@rust-highfive

This comment has been minimized.

@GuillaumeGomez GuillaumeGomez force-pushed the add-rustdoc-gui-tests branch from 6e9729e to ef95da7 Compare April 14, 2020 18:36
@rust-highfive

This comment has been minimized.

src/bootstrap/util.rs Outdated Show resolved Hide resolved
src/bootstrap/test.rs Outdated Show resolved Hide resolved
src/bootstrap/test.rs Outdated Show resolved Hide resolved
@crlf0710 crlf0710 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Apr 24, 2020
@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 2, 2020
@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 15, 2020
@GuillaumeGomez
Copy link
Member Author

Applied the comments, sorry for the long delay.

@GuillaumeGomez GuillaumeGomez force-pushed the add-rustdoc-gui-tests branch 2 times, most recently from b827068 to ea7d406 Compare May 26, 2020 15:30
@GuillaumeGomez GuillaumeGomez added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 31, 2020
libexpat1-dev \
gnupg \
apt-utils \
wget
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

src/ci/docker/x86_64-gnu-aux/Dockerfile Outdated Show resolved Hide resolved
src/bootstrap/test.rs Outdated Show resolved Hide resolved
@GuillaumeGomez GuillaumeGomez force-pushed the add-rustdoc-gui-tests branch from ea7d406 to ade95aa Compare June 6, 2020 11:20
@GuillaumeGomez
Copy link
Member Author

Updated.

@Mark-Simulacrum
Copy link
Member

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:

check-aux:
$(Q)$(BOOTSTRAP) test \
src/tools/cargo \
src/tools/cargotest \
$(BOOTSTRAP_ARGS)

By adding something like:

check-aux-and-gui: check-aux
	$(Q)$(BOOTSTRAP) test \
        src/test/rustdoc-gui \
		$(BOOTSTRAP_ARGS)

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.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 25, 2020
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-aux of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

 finished in 416.838
Build completed successfully in 0:37:19
    Finished dev [unoptimized + debuginfo] target(s) in 0.52s
Cloning into '/checkout/obj/build/browser-UI-test'...
Note: checking out 'e47b1a8697f628429cb684e0b3716737a3a0fa78'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.


If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

HEAD is now at e47b1a8... Merge pull request #152 from GuillaumeGomez/up-docker
Cloning into '/checkout/obj/build/test-rust-docs-ui'...
Note: checking out 'd74abb60d1bd1e3fbf647d2d2eb521f2aa357f27'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.


If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

HEAD is now at d74abb6... Merge pull request #84 from GuillaumeGomez/wait
    Finished release [optimized] target(s) in 0.21s
Copying stage0 std from stage0 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
Could not determine the LLVM submodule commit hash. Assuming that an LLVM rebuild is not necessary.
To force LLVM to rebuild, remove the file `/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/llvm-finished-building`
---
Copying stage1 rustc from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
Assembling stage2 compiler (x86_64-unknown-linux-gnu)
Uplifting stage1 std (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
Copying stage2 std from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
HEAD is now at e47b1a8... Merge pull request #152 from GuillaumeGomez/up-docker
HEAD is now at d74abb6... Merge pull request #84 from GuillaumeGomez/wait

> puppeteer@2.1.1 install /checkout/obj/build/browser-UI-test/node_modules/puppeteer
> node install.js


Chromium downloaded to /checkout/obj/build/browser-UI-test/node_modules/puppeteer/.local-chromium/linux-722234
added 164 packages from 138 contributors and audited 164 packages in 11.278s
found 14 low severity vulnerabilities
  run `npm audit fix` to fix them, or `npm audit` for details
    Finished release [optimized] target(s) in 0.21s
=> Starting doc-ui tests...

basic-code... ok
basic-code... ok
basic... ok
code-sidebar-toggle-mobile... ok
code-sidebar-toggle... ok
list_code_block... ok
module-mobile... ok
theme-change... FAILED (images "failures/theme-change-test.png" and "theme-change" are different)
toggle-docs... FAILED (images "failures/toggle-docs-test.png" and "toggle-docs" are different)

<= doc-ui tests done: 6 succeeded, 2 failed


command did not execute successfully: "/usr/bin/node" "../browser-UI-test/src/index.js" "--no-sandbox" "--test-folder" "ui-tests" "--failure-folder" "failures" "--variable" "DOC_PATH" "/checkout/obj/build/test-rust-docs-ui/test-docs/doc/test_docs" "--show-text" "--generate-images"


failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --stage 2 src/test/rustdoc-gui
Build completed unsuccessfully in 0:00:38
Build completed unsuccessfully in 0:00:38
make: *** [check-aux-and-gui] Error 1
Makefile:49: recipe for target 'check-aux-and-gui' failed
  local time: Wed Aug 26 16:55:48 UTC 2020
  network time: Wed, 26 Aug 2020 16:55:48 GMT
== end clock drift check ==
== end clock drift check ==
##[error]Process completed with exit code 2.
Terminate orphan process: pid (3953) (python)

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 @rust-lang/infra. (Feature Requests)

@bors
Copy link
Contributor

bors commented Aug 26, 2020

💔 Test failed - checks-actions

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Aug 26, 2020

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

@Mark-Simulacrum
Copy link
Member

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

@jyn514
Copy link
Member

jyn514 commented Aug 26, 2020

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

@GuillaumeGomez
Copy link
Member Author

The images generated in case of failures (the PNG screenshots).

@Mark-Simulacrum
Copy link
Member

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.

@GuillaumeGomez
Copy link
Member Author

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. :)

@Mark-Simulacrum
Copy link
Member

I am confused - you believe you have uploaded them? We don't upload by default from non dist builders at all...

@GuillaumeGomez
Copy link
Member Author

Oh snap... Any idea on how we could get access to the images generated then?

@Mark-Simulacrum
Copy link
Member

...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.

@GuillaumeGomez
Copy link
Member Author

I misundestood the issue then... Going to re-read it.

@crlf0710 crlf0710 marked this pull request as draft September 18, 2020 12:27
@bors
Copy link
Contributor

bors commented Sep 30, 2020

☔ 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:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@jyn514 jyn514 added the A-testsuite Area: The testsuite used to check the correctness of rustc label Sep 30, 2020
@Mark-Simulacrum
Copy link
Member

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!

@camelid camelid added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Dec 9, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 22, 2021
…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`
@GuillaumeGomez GuillaumeGomez deleted the add-rustdoc-gui-tests branch August 19, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants