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

feat(e2e): Add tests for the 'net' set of commands #826

Merged
merged 3 commits into from
Sep 19, 2023

Conversation

craciunoiuc
Copy link
Member

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran make fmt on your commit series before opening this PR;
  • Updated relevant documentation.

Description of changes

To run the tests you will need to be root or run with sudo because the commands manipulate network interfaces.

Hopefully this works in the actions, otherwise we can just try to skip them in the action.

@craciunoiuc craciunoiuc added kind/test Issue or PR is related to test(s) area/test Issue or PR is related to test(s) labels Sep 18, 2023
Copy link
Contributor

@antoineco antoineco left a comment

Choose a reason for hiding this comment

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

General comments:

  • All these commands are intertwined inside the test suites. inspect depends on up, down and rm for example. It seems like we could have easily covered most of them in a single suite.

  • It would have been interesting to test that a unikernel can actually make use of the network created by those commands (e.g. nginx from our catalog).

test/e2e/cli/net_rm_test.go Outdated Show resolved Hide resolved
test/e2e/cli/net_rm_test.go Outdated Show resolved Hide resolved
test/e2e/cli/net_up_test.go Outdated Show resolved Hide resolved
test/e2e/cli/net_down_test.go Outdated Show resolved Hide resolved
@craciunoiuc
Copy link
Member Author

Yes, for the first point, I wanted to use ifconfig to do what kraft does but then I realised that it's not really a good, generic way to test. So, better to just throw all commands in there and see where it breaks.

For the second point, it will surely be covered in the kraft run tests. Here we can at most add a ping to the bridge to see if it's up

@antoineco
Copy link
Contributor

antoineco commented Sep 19, 2023

Regarding the first point, what I meant is that you could pretty much keep only the file that's currently called net_inspect, and still cover almost all relevant functionalities.
If the goal is to test a lot of combinations of flags I agree that we need a tests suite for every subcommand. However I would focus only on the edge cases in these, to avoid the redundancy of the "happy path".

This is not a request for change btw, just an observation to reduce redundancy in the future. The code is written so let's keep it.

@craciunoiuc
Copy link
Member Author

Took a bit more to solve all stuff

I now suffixed every set of commands with the command name. Should make things a bit less confusing + less chance for errors because input/output won't (potentially) spill between commands

@craciunoiuc
Copy link
Member Author

aaand tests fail now online

@craciunoiuc
Copy link
Member Author

image

Even version fails, strange

@craciunoiuc
Copy link
Member Author

and now they seem to pass, wut

Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.io>
Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.io>
Signed-off-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.io>
@antoineco antoineco merged commit 263f80b into unikraft:staging Sep 19, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test Issue or PR is related to test(s) kind/test Issue or PR is related to test(s)
Projects
Status: 🚀 Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants