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

Run crates.io inside Travis (without Docker) #1825

Closed
wants to merge 1 commit into from

Conversation

kzys
Copy link
Contributor

@kzys kzys commented Sep 5, 2019

This is the first step towards #868.

Personally I still prefer #1814 (Docker version) over this one. We have docker-compose.yml for development, Procfile and buildpacks for production, and this shell script for CI, which might cause "works on my machine" problem.

@rust-highfive
Copy link

r? @sgrif

(rust_highfive has picked a reviewer for you, use r? to override)

@kzys kzys changed the title Run crates.io inside Travis (without Docker) [WIP] Run crates.io inside Travis (without Docker) Sep 5, 2019
@kzys
Copy link
Contributor Author

kzys commented Sep 5, 2019

@jtgeibel / @sgrif - Can we have a S3 bucket to share artifacts between jobs?

https://docs.travis-ci.com/user/build-stages/

It is important to note that jobs do not share storage, as each job runs in a fresh VM or container. If your jobs need to share files (e.g., using build artifacts from the “Test” stage for deployment in the subsequent “Deploy” stage), you need to use an external storage mechanism such as S3 and a remote scp server.

This change currently builds the Rust backend again on Integration Tests job.

@kzys kzys force-pushed the integ-test-without-docker branch 2 times, most recently from d8b8b0e to e98d724 Compare September 5, 2019 23:50
@kzys kzys changed the title [WIP] Run crates.io inside Travis (without Docker) Run crates.io inside Travis (without Docker) Sep 5, 2019
@kzys kzys mentioned this pull request Sep 6, 2019
@kzys
Copy link
Contributor Author

kzys commented Oct 8, 2019

Welcome back @carols10cents! Can you take a look this PR, since you are the one who had opened #868?

@carols10cents
Copy link
Member

Hi @kzys! Thank you for your patience! I will get to this once I've finished with the PRs that have been open longer than this one has.

Copy link
Member

@jtgeibel jtgeibel left a comment

Choose a reason for hiding this comment

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

I've added some review comments. At a high level, I'd also like to see a more thorough integration test implemented, and a clearer path to coordinating multiple tests. Right now I suspect this test would pass even if we didn't boot the backend server. I think we'll need way to load test fixtures into the database, and verify the HTML contains the expected data.

Basically, I had envisioned that integration tests would probably require us writing some sort of framework that would coordinate all of this, and I'm not sure that a shell script will be sufficient long term. Of course, we'll need to start somewhere and take incremental steps to get to that point, but I'm not sure I see the advantage of merging just this current test at this point and running it on CI for all future PRs. I am however open to leaving this PR open as a place where we can experiment with building out this infrastructure further, I just don't expect that we would be merging this until it is in a state that it will catch potential problems.

I hope I'm not coming off as pessimistic here. I expect that we won't really start to see the advantage of such integration tests until we get Fastboot enabled and then implemented on some of the dynamic endpoints. I'd like to get back into reviewing the Fastboot stuff soon, and I definitely see integration tests providing lots of value in that context.

.travis.yml Outdated Show resolved Hide resolved
@@ -62,6 +63,16 @@ matrix:
- npm run lint:js
- npm run lint:deps
- npm test
- name: Integration Tests
language: node_js
Copy link
Member

Choose a reason for hiding this comment

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

Even though we have cache.cargo: true above, it looks like this job isn't actually caching any of our Rust build artifacts. I recently realized that travis seems to use different VM images depending on the selected language type. It seems like other behavior, like caching here, must also be dependent on the language type.

This brings me to a general thought. We previously run the frontend tests within our rust: stable job. It seems like it might be cleaner to combine these back into a single job and run any integration tests there as well. That would reduce the duplication of running the split jobs and duplicating the build work from both of those in this integration job. The downside is that I'm starting to see that Travis doesn't really seem to be set up to gracefully handle multiple languages/frameworks within the same job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally prefer the separation. We tend to have either JavaScript-only or Rust-only PRs. The separation helps developers understand what's not working - and which test failures are false-positives.

Between stages, Travis doesn't share any files. Can't we have a S3 bucket to keep intermediate artifacts?

https://docs.travis-ci.com/user/build-stages/#data-persistence-between-stages-and-jobs

Copy link
Member

Choose a reason for hiding this comment

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

I don't have access to create an S3 bucket for intermediate artifacts, but I've added it to the agenda at rust-lang/crates-io-cargo-teams#60 to discuss at the next team meeting.

script/ci/start-frontend-and-backend.sh Outdated Show resolved Hide resolved
script/ci/start-frontend-and-backend.sh Outdated Show resolved Hide resolved
script/ci/start-frontend-and-backend.sh Outdated Show resolved Hide resolved
To test the frontend and the backend together, this change adds
a new Travis job "Integration Tests".
@kzys
Copy link
Contributor Author

kzys commented Nov 15, 2019

Another path we could take is deploying to Heroku, as a part of integration testing.

Pros:

  • We don't have to worry about artifact sharing.
  • It is closer to production and we can even test Nginx parts.
  • Looking test failures would be easier, since developers can access the instance directly from their browsers.

Cons:

  • We cannot do that per-PR basis. The deployment may be happening only from master.
  • We need to have an isolated database instance for testing.

What do you think?

@kzys
Copy link
Contributor Author

kzys commented Nov 19, 2019

Let me close this PR for now, since I would not work on that in 2019.

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.

None yet

5 participants