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 integration tests. #873

Merged
merged 1 commit into from
Jul 3, 2022
Merged

Add integration tests. #873

merged 1 commit into from
Jul 3, 2022

Conversation

Alexhuszagh
Copy link
Contributor

@Alexhuszagh Alexhuszagh commented Jun 27, 2022

Adds an an integration test for remote Docker support with and without persistent data volumes, and adds it to CI. In addition, we've added integration tests for custom toolchains using cargo-bisect-rustc. Also changes the workflow to ensure publish depends on fmt, clippy, and cargo-deny. Fixes a bug when removing data volumes, since it does not require a target.

@Alexhuszagh Alexhuszagh added meta issues/PRs related to the maintenance of the crate. no changelog A valid PR without changelog (no-changelog) labels Jun 27, 2022
@Alexhuszagh Alexhuszagh force-pushed the integration branch 4 times, most recently from ce51acc to 8889cf0 Compare June 27, 2022 03:37
@Alexhuszagh Alexhuszagh changed the title Add integration tests for remote docker support. Add integration tests. Jun 27, 2022
@Alexhuszagh
Copy link
Contributor Author

Since this would slow down our CI by quite a bit by default (bisect took 8 min, remote took 4 min), while most of our other actions take ~1m or less, I've changed it so it only runs if we use bors, so this works on PRs or if we try. (I'm using an invalid target here intentionally).

bors try --target x

bors bot added a commit that referenced this pull request Jun 27, 2022
@Alexhuszagh

This comment was marked as outdated.

@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Jun 27, 2022
@bors

This comment was marked as outdated.

@Alexhuszagh

This comment was marked as resolved.

bors bot added a commit that referenced this pull request Jun 27, 2022
@Alexhuszagh Alexhuszagh marked this pull request as ready for review June 27, 2022 03:53
@Alexhuszagh Alexhuszagh requested a review from a team as a code owner June 27, 2022 03:53
@bors

This comment was marked as outdated.

@Alexhuszagh Alexhuszagh marked this pull request as draft June 27, 2022 03:58
@Alexhuszagh

This comment was marked as outdated.

@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Jun 27, 2022
@bors

This comment was marked as outdated.

@Alexhuszagh Alexhuszagh marked this pull request as ready for review June 27, 2022 16:14
@Alexhuszagh Alexhuszagh force-pushed the integration branch 2 times, most recently from 9af699c to ab3fe77 Compare June 27, 2022 17:36
@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Jun 27, 2022
@bors

This comment was marked as outdated.

@Alexhuszagh
Copy link
Contributor Author

https://app.codecov.io/gh/cross-rs/cross/compare/873/tree/src/docker/shared.rs#D1L891 see coverage

Ah that's unusual. Not sure why that's the case, it should be defining HOSTNAME inside the image. I guess the diagnostics are good, then.

@Emilgardis
Copy link
Member

Aha, I get it now!

You'll have to forward RUSTFLAGS inside the containered container, it's probably doing everything correct just not providing coverage.

See https://github.com/cross-rs/cross/runs/7169840511?check_suite_focus=true#step:19:22 where there's no profraw files and https://github.com/cross-rs/cross/runs/7169840371?check_suite_focus=true#step:10:7 where there is

@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Jul 3, 2022
@bors

This comment was marked as outdated.

@Emilgardis
Copy link
Member

hmm, still not working

      Running `CARGO_INCREMENTAL=0 /home/runner/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-profdata merge -sparse -o /home/runner/work/cross/cross/target/cross.profdata`
added 18 packages, and audited 19 packages in 1s
2 packages are looking for funding
  run `npm fund` for details
found 0 vulnerabilities
added 3 packages, changed 3 packages, and audited 22 packages in 360ms
3 packages are looking for funding
  run `npm fund` for details
found 0 vulnerabilities
error: no input files specified. See llvm-profdata merge -help

I don't know why.

@Alexhuszagh
Copy link
Contributor Author

I have no idea either, I might need to also forward all of these as well:

  • LLVM_PROFILE_FILE
  • CARGO_INCREMENTAL
  • RUST_TEST_THREADS

@Emilgardis
Copy link
Member

oh.. ofc, need to forward LLVM_PROFILE_FILE too...

@Emilgardis
Copy link
Member

Incremental would be good to disable, as we don't use the incremental data

@Alexhuszagh
Copy link
Contributor Author

Incremental would be good to disable, as we don't use the incremental data

I've just forwarded everything, since if they're provided by the action, there's probably a good reason for them.

bors try --target aarch64-unknown-linux-gnu

bors bot added a commit that referenced this pull request Jul 3, 2022
@bors

This comment was marked as outdated.

@Alexhuszagh
Copy link
Contributor Author

Code coverage still isn't working. At least the code tests to work on my machine.

@Emilgardis
Copy link
Member

Emilgardis commented Jul 3, 2022

We need to make the target folder available at the same path/the path in LLVM_PROFILE, easiest solution is to do -v /home/runner/work/cross/cross/target/:/home/runner/work/cross/cross/target/

edit: if we also do -v /home/runner/work/cross/cross/target/:/project/target we will get cache

@Emilgardis Emilgardis self-requested a review July 3, 2022 17:53
@Alexhuszagh
Copy link
Contributor Author

bors try --target aarch64-unknown-linux-gnu

bors bot added a commit that referenced this pull request Jul 3, 2022
@bors
Copy link
Contributor

bors bot commented Jul 3, 2022

try

Build succeeded:

@Alexhuszagh
Copy link
Contributor Author

Nothing is working so I'm going to throw in a panic and see if it just "works".

@Alexhuszagh
Copy link
Contributor Author

Let's see if it panics:
bors try --target aarch64-unknown-linux-gnu

bors bot added a commit that referenced this pull request Jul 3, 2022
@Emilgardis
Copy link
Member

It is working, no? https://github.com/cross-rs/cross/runs/7170509736

@Alexhuszagh
Copy link
Contributor Author

It is working, no? https://github.com/cross-rs/cross/runs/7170509736

Wait it is, maybe the code coverage I got was outdated. I'll squash the commits into one.

Adds an an integration test for remote Docker support with and without persistent data volumes, and adds it to CI. In addition, we've added integration tests for custom toolchains using `cargo-bisect-rustc`, and tests for docker-in-docker scenarios. Also changes the workflow  to ensure publish depends on fmt, clippy, and cargo-deny. Fixes a bug  when removing data volumes, since it does not require a target.

The docker-in-docker scenarios both run the cross unittests, and test workspace and manifest paths.
@bors
Copy link
Contributor

bors bot commented Jul 3, 2022

try

Build succeeded:

Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

bors r+

awesome

@bors
Copy link
Contributor

bors bot commented Jul 3, 2022

Build succeeded:

@bors bors bot merged commit 4f0b147 into cross-rs:main Jul 3, 2022
@Alexhuszagh Alexhuszagh deleted the integration branch July 3, 2022 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta issues/PRs related to the maintenance of the crate. no changelog A valid PR without changelog (no-changelog)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants