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

Rename tests requiring root #451

Merged
merged 2 commits into from
May 18, 2023
Merged

Conversation

danielocfb
Copy link
Collaborator

We have some complicated and non-obvious logic for skipping certain
tests when running cargo test from the root of the crate in CI, but then
an explicit invocation from the libbpf-rs/ sub-directory attempting to
run those skipped tests (now under sudo).
Simplify the logic and hopefully make it somewhat more obvious by
prefixing all tests that require root with test_root_.

Note: this is a workaround for what I'd call a bug in Cargo. I opened
rust-lang/cargo#12147 in the hopes that this
will be addressed eventually (though I don't know how realistic that
is). If we are still getting confusion even with this more explicit
naming, we may want to consider moving (or copying) libbpf-rs's .cargo/
directory into the workspace root.

@danielocfb danielocfb requested a review from anakryiko May 17, 2023 17:38
Copy link
Member

@anakryiko anakryiko left a comment

Choose a reason for hiding this comment

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

naming nit: "root" is a bit ambiguous (it could be interpreted as "root of file hierarchy", so "test_sudo_" would be better, IMO. But I don't feel strongly about this, so up to you.

Let's co-locate the map_info test with the remaining end-to-end tests
for easier management.

Signed-off-by: Daniel Müller <deso@posteo.net>
We have some complicated and non-obvious logic for skipping certain
tests when running cargo test from the root of the crate in CI, but then
an explicit invocation from the libbpf-rs/ sub-directory attempting to
run those skipped tests (now under sudo).
Simplify the logic and hopefully make it somewhat more obvious by
prefixing all tests that require root with `test_sudo_`.

Note: this is a workaround for what I'd call a bug in Cargo. I opened
rust-lang/cargo#12147 in the hopes that this
will be addressed eventually (though I don't know how realistic that
is). If we are still getting confusion even with this more explicit
naming, we may want to consider moving (or copying) libbpf-rs's .cargo/
directory into the workspace root.

Signed-off-by: Daniel Müller <deso@posteo.net>
@danielocfb
Copy link
Collaborator Author

naming nit: "root" is a bit ambiguous (it could be interpreted as "root of file hierarchy", so "test_sudo_" would be better, IMO. But I don't feel strongly about this, so up to you.

Sure, I changed the prefix.

@danielocfb danielocfb merged commit 319fc3b into libbpf:master May 18, 2023
@danielocfb danielocfb deleted the topic/map-info-test branch May 18, 2023 20:10
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.

3 participants