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 basic smoke tests for docs.rs #524

Closed
wants to merge 21 commits into from
Closed

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Dec 15, 2019

After the fiasco with #519, I figured I'd write this before making any new PRs.

This is very much an integration test, not a unit test. It should catch things like 'docs.rs doesn't build with an empty .rustwide cache', but because of that, it takes a very long time to run (~45 minutes on Github Actions). I tried to set up caching, but the permissions weren't working quite right: actions/cache#131

This runs reasonably quickly locally, ~5 minutes if everything is cached.

This depends on the changes from #523 (because otherwise the tests take about 25 minutes locally). Either that PR should be merged first or it should be closed when this one is merged.

Finally, this also uncomments two #[ignore] tests that were passing. The rest are left commented since they're currently failing.

There should be no change in behavior to the production server, since it doesn't run builds with -s anyway.

r? @pietroalbini

greatly speeds up builds using --skip + docker-compose, for which the local
cache will _always_ be empty
this is the quick and dirty way, but it's better than having no tests at
all.
This shouldn't affect the CI at all since it has a new image every time,
but greatly speeds up local development.
should speed up builds significantly
docker doesn't like it when you change its files while it's running
also uses sh instead of bash since the test file turned out to be
portable
doesn't preserve ownership or write permissions
@pietroalbini
Copy link
Member

So, while as a stopgap solution I feel this is ok, I don't want to rely on this too long:

  • To test the builder works, we shouldn't rely on real world crates, as they take too long to build. For rustwide and Crater I created dummy crates specific to the tests I was writing, for example rustwide's out-of-memory crate.
  • To test the webserver works, we shouldn't build real crates, but just "simulate" builds by adding stuff to the database directly.
  • We should move away from bash scripts, and integrate the whole thing with cargo test.

Also, fyi, uploading the cache failed. If you remove the caching from GitHub (also because we're surely over the 400MB limit) we can merge this.

this went way over github's limits and was failing the build.
@jyn514
Copy link
Member Author

jyn514 commented Dec 16, 2019

Removed the cache.

To test the builder works, we shouldn't rely on real world crates, as they take too long to build. For rustwide and Crater I created dummy crates specific to the tests I was writing, for example rustwide's out-of-memory crate.

The longest part of the build seems to actually be initializing Rustwide, not building the crates. I think building dummy crates will help, but I'm not sure by how much.

To test the webserver works, we shouldn't build real crates, but just "simulate" builds by adding stuff to the database directly.

That's a really good idea.

We should move away from bash scripts, and integrate the whole thing with cargo test.

+1

@pietroalbini
Copy link
Member

The longest part of the build seems to actually be initializing Rustwide, not building the crates. I think building dummy crates will help, but I'm not sure by how much.

Is WorkspaceBuilder::fast_init called when executing tests? We shouldn't enable it on production, but when running tests it should help a lot. rustwide's CI takes ~11 minutes to initialize a workspace and run tests in it.

@jyn514
Copy link
Member Author

jyn514 commented Dec 19, 2019

Is WorkspaceBuilder::fast_init called when executing tests?

No, because this doesn't change any of the code. Maybe we could set fast_init based on an environment variable? That would also be useful for local development.

@pietroalbini
Copy link
Member

Maybe we could set fast_init based on an environment variable? That would also be useful for local development.

Yeah.

@jyn514
Copy link
Member Author

jyn514 commented Dec 21, 2019

Added fast_init. It still takes ~45 minutes :/

@jyn514 jyn514 mentioned this pull request Jan 8, 2020
2 tasks
@pietroalbini
Copy link
Member

Can we convert this to the new testing tools?

@jyn514
Copy link
Member Author

jyn514 commented Jan 8, 2020

Yes, I can do that sometime this week.

@jyn514 jyn514 self-assigned this Jan 8, 2020
@jyn514
Copy link
Member Author

jyn514 commented Jan 23, 2020

Making a list of things to do:

@jyn514 jyn514 mentioned this pull request Jan 24, 2020
13 tasks
@jyn514
Copy link
Member Author

jyn514 commented Jan 24, 2020

Closing in favor of #570

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.

2 participants