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

integration-test: Implement running on VMs #733

Merged
merged 2 commits into from
Aug 9, 2023
Merged

Conversation

tamird
Copy link
Member

@tamird tamird commented Aug 9, 2023

See individual commits.


This change is Reviewable

@netlify
Copy link

netlify bot commented Aug 9, 2023

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 82a77bc
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/64d3eb97ce9295000804ab09
😎 Deploy Preview https://deploy-preview-733--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mergify mergify bot added aya-log Relating to aya-log test A PR that improves test cases or CI labels Aug 9, 2023
Copy link
Member

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

My biggest feedback about this PR at this point is that there's a lot of context in your brain about how these pieces fit together that aren't really apparent without unpacking the structure of the code.

In particular, a doc comment on the init process would be good. Also good would be a comment in xtask::run with a high-level overview of what's going on.

Thanks for making this all work on the mac! Very handy

Copy link
Member Author

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Done.

Reviewable status: 0 of 11 files reviewed, all discussions resolved (waiting on @dave-tucker)

Copy link
Member

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

This is looking great! Pure Rust 🦀 and few external deps.
Thanks for doing this - looks like it will be pretty easy to build upon.

A couple of nits and things that could use some commenting for coherence.

init/src/main.rs Show resolved Hide resolved
init/src/main.rs Show resolved Hide resolved
init/src/main.rs Outdated Show resolved Hide resolved
init/src/main.rs Show resolved Hide resolved
xtask/src/run.rs Show resolved Hide resolved
xtask/src/run.rs Show resolved Hide resolved
xtask/src/run.rs Outdated Show resolved Hide resolved
xtask/src/run.rs Outdated Show resolved Hide resolved
@@ -72,6 +74,7 @@ lazy_static = { version = "1", default-features = false }
libc = { version = "0.2.105", default-features = false }
log = { version = "0.4", default-features = false }
netns-rs = { version = "0.1", default-features = false }
nix = { version = "0.26.2", default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

I for one am very pro nix and have 0 objections to using it in xtask.
ISTR in any we only used libc and I don't think we need to be as strict, but flagging it anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

xtask/src/run.rs Show resolved Hide resolved
Wait for at least one log and increase the wait time 10x.
Copy link
Member

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

Yes, the comments please. Thank you.

Reviewable status: 0 of 11 files reviewed, 12 unresolved discussions (waiting on @dave-tucker and @tamird)

Copy link
Member Author

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Thanks for the review!

init/src/main.rs Show resolved Hide resolved
init/src/main.rs Show resolved Hide resolved
init/src/main.rs Show resolved Hide resolved
init/src/main.rs Outdated Show resolved Hide resolved
xtask/src/run.rs Show resolved Hide resolved
xtask/src/run.rs Outdated Show resolved Hide resolved
xtask/src/run.rs Show resolved Hide resolved
xtask/src/run.rs Show resolved Hide resolved
xtask/src/run.rs Show resolved Hide resolved
line.with_context(|| format!("failed to read line from {qemu:?}"))?;
println!("{}", line);
if let Some(line) = line.strip_prefix("init: ") {
let previous = match line {
Copy link
Member Author

Choose a reason for hiding this comment

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

Can you be more specific?

Implements running integration tests on multiple VMs with arbitrary
kernel images using `cargo xtask integration-test vm ...`.

This changes our coverage from 6.2 to 6.1 and 6.4.
Copy link
Member

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

LGTM on green ✅

Copy link
Member Author

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 11 files reviewed, 12 unresolved discussions (waiting on @dave-tucker)

a discussion (no related file):

Previously, dave-tucker (Dave Tucker) wrote…

I missed the parens when reviewing. I see them now.

Done.


a discussion (no related file):

Previously, dave-tucker (Dave Tucker) wrote…

If it's useful we keep it.

Done.


@tamird tamird merged commit 412a087 into aya-rs:main Aug 9, 2023
21 checks passed
@tamird tamird deleted the kernel-test branch August 9, 2023 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aya-log Relating to aya-log test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants