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 network container tests #11583

Merged
merged 1 commit into from
Jan 16, 2023
Merged

Add network container tests #11583

merged 1 commit into from
Jan 16, 2023

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jan 15, 2023

This adds some tests which use Docker containers to provide HTTPS and SSH servers. This should help with validating that Cargo's networking and security are working correctly. It can also potentially be used in the future for other tests that require more complex setups.

These tests are only run on Linux in CI. macOS does not have Docker there, and the Windows Docker does not support Linux containers. The tests should work on macOS if you run them locally with Docker Desktop installed. The SSH tests do not work on Windows due to issues with ssh-agent, but the HTTPS tests should work with Docker Desktop.

These tests require an opt-in environment variable to run:

  • CARGO_PUBLIC_NETWORK_TESTS=1 — This is for tests that contact the public internet.
  • CARGO_CONTAINER_TESTS=1 — This is for tests that use Docker.

@rustbot
Copy link
Collaborator

rustbot commented Jan 15, 2023

r? @epage

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 15, 2023
Comment on lines +231 to +242
remove_if_exists(&self.name);
}
}

fn remove_if_exists(name: &str) {
if let Err(e) = Command::new("docker")
.args(&["container", "rm", "--force", name])
.output()
{
panic!("failed to run docker: {e}");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

While this is test code, I thought panic during Drop is problematic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, it should generally always be avoided. In this particular case, I wasn't too concerned because the panic should be extremely unlikely. The only way for it to happen in the drop is if it fails to launch docker, but that should only happen if it isn't in PATH (which can't happen because the handle won't be created otherwise), or if the system is running out of memory or file descriptors (in which case you've probably got bigger problems). I was more interested in sharing this function in the two places it is used and to keep the code simple. I can make it return a Result and move the panic to the other location if you think it would make that clearer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Meh. I leave it to you. I'll merge as-is and we can always iterate

Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

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

Looks good. Before merging, just wanted to double check your thoughts on the Drop

@epage
Copy link
Contributor

epage commented Jan 16, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jan 16, 2023

📌 Commit 4cb9ac3 has been approved by epage

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2023
@bors
Copy link
Contributor

bors commented Jan 16, 2023

⌛ Testing commit 4cb9ac3 with merge a5d47a7...

@bors
Copy link
Contributor

bors commented Jan 16, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing a5d47a7 to master...

@bors bors merged commit a5d47a7 into rust-lang:master Jan 16, 2023
weihanglo added a commit to weihanglo/rust that referenced this pull request Jan 17, 2023
9 commits in 1cd6d3803dfb0b342272862a8590f5dfc9f72573..a5d47a72595dd6fbe7d4e4f6ec20dc5fe724edd1
2023-01-12 18:40:36 +0000 to 2023-01-16 18:51:50 +0000

- Add network container tests (rust-lang/cargo#11583)
- Show progress of crates.io index update even `net.git-fetch-with-cli` option enabled (rust-lang/cargo#11579)
- `cargo metadata` supports artifact dependencies (rust-lang/cargo#11550)
- fix(docs): add required "inherits" option to example profile (rust-lang/cargo#11504)
- add documentation that SSH markers aren't supported (rust-lang/cargo#11586)
- Fix typo (rust-lang/cargo#11585)
- Enable source_config_env test on Windows (rust-lang/cargo#11582)
- Support `codegen-backend` and `rustflags` in profiles in config file (rust-lang/cargo#11562)
- ci: reflect to clap updates (rust-lang/cargo#11578)
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2023
Update cargo

9 commits in 1cd6d3803dfb0b342272862a8590f5dfc9f72573..a5d47a72595dd6fbe7d4e4f6ec20dc5fe724edd1 2023-01-12 18:40:36 +0000 to 2023-01-16 18:51:50 +0000

- Add network container tests (rust-lang/cargo#11583)
- Show progress of crates.io index update even `net.git-fetch-with-cli` option enabled (rust-lang/cargo#11579)
- `cargo metadata` supports artifact dependencies (rust-lang/cargo#11550)
- fix(docs): add required "inherits" option to example profile (rust-lang/cargo#11504)
- add documentation that SSH markers aren't supported (rust-lang/cargo#11586)
- Fix typo (rust-lang/cargo#11585)
- Enable source_config_env test on Windows (rust-lang/cargo#11582)
- Support `codegen-backend` and `rustflags` in profiles in config file (rust-lang/cargo#11562)
- ci: reflect to clap updates (rust-lang/cargo#11578)

r? `@ghost`
@ehuss ehuss added this to the 1.68.0 milestone Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants