-
Notifications
You must be signed in to change notification settings - Fork 10
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
Added e2e tests for untagged and any new EVCs #210
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Alopalao great e2e cov.
- Did you also run this locally?
Yes, the results are posted on mef_eline PR too,
|
tests/test_e2e_10_mef_eline.py
Outdated
h11.cmd('ip addr add 100.0.0.11/24 dev vlan100') | ||
h2.cmd('ip link add link %s name vlan100 type vlan id 100' % (h2.intfNames()[0])) | ||
h2.cmd('ip link set up vlan100') | ||
h2.cmd('ip addr add 100.0.0.2/24 dev vlan100') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
symmetric hosts VLANs I think it's fair game to test this use case, but let's also assert the installed UNI flows on the switches based on your spreadsheet for completeness. @italovalcy let us know if you'd like to see this tested differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it is worth adding assertions on the flows besides doing the ping test. Especially since we don't have tests for asymmetric VLANs so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I divided the PR, here is the correct one for mef_eline
Nice. |
tests/test_e2e_10_mef_eline.py
Outdated
time.sleep(10) | ||
|
||
h11, h2 = self.net.net.get('h11', 'h2') | ||
h11.cmd('ip link add link %s name vlan100 type vlan id 100' % (h11.intfNames()[0])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are using the "any" VLAN here, what do you think about using a random value for the VLAN on the PING test? I suggest replacing the VLAN 100 with a random value chosen from a range from 1 to 4095. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I divided the PR, here is the correct one for mef_eline
@Alopalao I've closed the threads that I opened. Thanks for pushing these updates. Other than the threads that you're aligning with Italo, when it's done, please rerun e2e selecting flow_manager related tests. Also, another piece of advice will be to break this PR in two, since mef_eline PR is still in review, if you split it in two, covering flow_manager here and mef_eline on another one then of_core PR kytos-ng/of_core#98 could land soon as well once this PR here is approved. You could still keep both but it will sit longer in code review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'll leave this PR approved since Aldo managed to split the PRs to simplify the review and the open discussions will continue on PR #213
@Alopalao, can you rerun one last time test_032_on_switch_reconnection_should_recreate_untagged_any_flows
to ensure it doesn't have any auto-rerun from pytest?
New build test results:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for sending this PR to cover the changes on flow_manger and of_core, Aldo! Very well done.
Related to mef_eline and of_core PRs
Summary
Tests including the recent addition to
vlan
available valuesuntagged
andany
.Local Tests