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

Add GUI tests #1698

Merged
merged 7 commits into from
Nov 16, 2023
Merged

Add GUI tests #1698

merged 7 commits into from
Nov 16, 2023

Conversation

GuillaumeGomez
Copy link
Member

Part of #1494.

I finally took the time to wrap something together. So in this case, I took what we do for rustdoc and simply ported it to docs.rs. I added 2 GUI tests so you can take a look at what they look like. The documentation for the framework is here.

Do you think this is the right approach or would you prefer another framework?

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@GuillaumeGomez GuillaumeGomez force-pushed the gui-tests branch 3 times, most recently from 7a56fd2 to 2f2d4a7 Compare March 28, 2022 19:16
@GuillaumeGomez
Copy link
Member Author

I'm a bit stuck with the current docker situation. So to sum up what I tried to and what my issue is:

  • I want to have a docs.rs environment set up with a "clean" database with only the crates/updates we need for the GUI tests. The other point is that it doesn't "pollute" the existing DB by adding new crates.
  • It should be reproducible easily.
  • The docker image doesn't need to be rebuilt every time you make a GUI test update.

So now the problem is that I think putting everything in one docker container isn't a great idea because it makes a big image which needs to be rebuilt every time there is a change on docs.rs source code.

The other problem is that apparently, we can't start docs.rs web server in the container: it exists right away (without an error).

So first: is it the right approach? Secondly: any help would be very appreciated.

@GuillaumeGomez GuillaumeGomez force-pushed the gui-tests branch 2 times, most recently from 238be21 to 744a2f2 Compare April 12, 2022 22:14
@GuillaumeGomez
Copy link
Member Author

So I was finally able to make it work like I wanted. I even added it to the docker-compose and I added a shell script to make it simpler to run.

@GuillaumeGomez GuillaumeGomez force-pushed the gui-tests branch 5 times, most recently from de2cdce to 6048c15 Compare April 13, 2022 13:55
@GuillaumeGomez
Copy link
Member Author

Maybe ping @Nemo157 ?

@syphar syphar added the S-waiting-on-author Status: This PR is incomplete or needs to address review comments label Sep 17, 2022
@GuillaumeGomez GuillaumeGomez removed the S-waiting-on-author Status: This PR is incomplete or needs to address review comments label Sep 24, 2022
@GuillaumeGomez
Copy link
Member Author

@syphar It's "ready" now, we need to discuss about if this looks good to you or if it seems to complicated to maintain and eventually try to pick another solution.

@syphar
Copy link
Member

syphar commented Sep 30, 2022

@syphar It's "ready" now, we need to discuss about if this looks good to you or if it seems to complicated to maintain and eventually try to pick another solution.

Thanks for the ping!
This topic was next on my list to dig into.

For my understanding: rustdoc is actively using the same framework?

@GuillaumeGomez
Copy link
Member Author

Yes. You can see it being used in src/test/rustdoc-gui.

@GuillaumeGomez
Copy link
Member Author

I updated to the last version of the framework and added some documentation in case you want to run it locally. So with this, it should be ready.

From here, the plan is then to test as much as possible our UI, especially the top-navbar and all docs.rs pages.

@syphar syphar self-requested a review October 17, 2023 15:19
@syphar syphar added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Oct 17, 2023
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
gui-tests/tester.js Show resolved Hide resolved
dockerfiles/run-gui-tests.sh Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

CI finally passed! \o/

PR is now ready. :)

@syphar syphar added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Nov 15, 2023
Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only one last comment from my side. I wonder if we can safe time on the setup, or if caching will help a a little, 20 minutes, are quite long.

I would like to have a short test from @Nemo157 because I can't currently run the tests on my mac

dockerfiles/run-gui-tests.sh Show resolved Hide resolved
@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Nov 15, 2023
@GuillaumeGomez
Copy link
Member Author

only one last comment from my side. I wonder if we can safe time on the setup, or if caching will help a a little, 20 minutes, are quite long.

Yes, 20 minutes is way too long. If you know how to cache it, I'd be very happy to do it.

I would like to have a short test from @Nemo157 because I can't currently run the tests on my mac

Is it because of this PR or something specific to your mac?

@syphar
Copy link
Member

syphar commented Nov 15, 2023

only one last comment from my side. I wonder if we can safe time on the setup, or if caching will help a a little, 20 minutes, are quite long.

Yes, 20 minutes is way too long. If you know how to cache it, I'd be very happy to do it.

do you know where most of the time is spent?

I would like to have a short test from @Nemo157 because I can't currently run the tests on my mac

Is it because of this PR or something specific to your mac?

I mean these GUI tests don't run, the other tests do.

the -- build crate ... things don't run, and I have to use docker compose run web build crate...

but running the test script leads to another error related to the host / container platform (I'm on an M1).

But IMO this doesn't block this PR, I'll figure it out afterwards.

@GuillaumeGomez
Copy link
Member Author

Oh wow. It took only 8 minutes without docker build.

@Nemo157
Copy link
Member

Nemo157 commented Nov 15, 2023

the -- build crate ... things don't run, and I have to use docker compose run web build crate...

I also can't use a local build crate, the rustup that rustwide downloads doesn't like nixos.

@GuillaumeGomez
Copy link
Member Author

Wow, weird. I thought using docker would prevent all these issues. ^^'

@Nemo157
Copy link
Member

Nemo157 commented Nov 15, 2023

rustwide does all the toolchain installation outside docker containers for some reason (it should be possible to move that all into the containers, which would probably make it work on macos just fine too).

@GuillaumeGomez
Copy link
Member Author

I see. Do you want it to be fixed before we merge this PR or after?

@Nemo157
Copy link
Member

Nemo157 commented Nov 15, 2023

It's orthogonal to this PR (and given maintenance status of rustwide unlikely to happen soon). I plan to see if I can adapt the script here to run everything inside docker-compose containers, can probably get around to it today if it's not too complicated.

@GuillaumeGomez
Copy link
Member Author

Thanks! :D

@Nemo157
Copy link
Member

Nemo157 commented Nov 15, 2023

I got it working in 16d4957 and managed to run them locally, but I think this can be much simpler after the refactoring in #2269, so I'd merge as is and I'll try to finish that up and update the script later.

@GuillaumeGomez
Copy link
Member Author

I'll let you handle the merge when you're ready then. Thanks for the help!

@syphar
Copy link
Member

syphar commented Nov 16, 2023

@Nemo157 so you think we should merge this PR now, and then improve on #2269 ?

@Nemo157
Copy link
Member

Nemo157 commented Nov 16, 2023

Yep

@syphar syphar merged commit e1be43f into rust-lang:master Nov 16, 2023
8 checks passed
@github-actions github-actions bot added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Nov 16, 2023
@GuillaumeGomez GuillaumeGomez deleted the gui-tests branch November 16, 2023 09:46
@GuillaumeGomez
Copy link
Member Author

Awesome! \o/

@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants