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

restructure the setup and teardown functions for network.py scripts (Bugfix) #1342

Merged
merged 19 commits into from
Sep 9, 2024

Conversation

stanley31huang
Copy link
Collaborator

@stanley31huang stanley31huang commented Jul 18, 2024

Description

There are some improvement in this PR as following:

  1. restructure the setup and teardown functions with context manager library.
  2. not bring up the network interfaces if it was down before testing.
  3. not shutdown the conduit network interface (master network interface) while testing with an user network interface (slave network interface) (for Linux DSA network)

Resolved issues

#233
#401

Documentation

Reference:

Tests

Tested on system with multiple physical network

Tested on system with DSA network

Copy link

codecov bot commented Jul 18, 2024

Codecov Report

Attention: Patch coverage is 92.50000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 45.99%. Comparing base (b92edc5) to head (8fac712).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
providers/base/bin/network.py 92.50% 4 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1342      +/-   ##
==========================================
+ Coverage   45.72%   45.99%   +0.26%     
==========================================
  Files         367      367              
  Lines       39134    39184      +50     
  Branches     6618     6629      +11     
==========================================
+ Hits        17894    18021     +127     
+ Misses      20565    20482      -83     
- Partials      675      681       +6     
Flag Coverage Δ
provider-base 20.66% <91.66%> (+0.74%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stanley31huang stanley31huang marked this pull request as ready for review July 22, 2024 06:20
@stanley31huang stanley31huang changed the title restructure the setup and teardown functions (Bugfix) restructure the setup and teardown functions for network.py scripts (Bugfix) Aug 5, 2024
restructure the setup and teardown functions with context manager
library. Another improvement is not bring up the network interfaces if it
was down before testing.
fix issue that the wait_for_iface_up function will not return correct status
correct the function name for checking the underspeed issue
fix the ruturn value of check_underspeed function
fix check_underspeed
update error messages
correct the condition to shutdown all other netifs
correct conduit network interface
update variable name
revise variable name
fix calledprocess error
add unittest for network.py
fix unittest failed on python3.5
add more unit tests
fix unit test
Copy link
Collaborator

@Hook25 Hook25 left a comment

Choose a reason for hiding this comment

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

For me it is very hard to understand if this is ok actually, I have a few nitpicks about the code but nothing major. The submissions you provide actually have 2 failed tests, so I'm not sure what is going on there. Could this just randomly fail on some machines? Did you test on just two? Can you please test this on more? This is not ran during sru right?

(Oh and fantastic job with the context manager, I really like that!)

providers/base/bin/network.py Outdated Show resolved Hide resolved
providers/base/bin/network.py Outdated Show resolved Hide resolved
providers/base/bin/network.py Outdated Show resolved Hide resolved
stanley31huang and others added 2 commits September 2, 2024 10:53
Co-authored-by: Massimiliano <massimiliano.girardi@canonical.com>
fix issue
@fernando79513 fernando79513 added the waiting-for-changes The review has been completed but the PR is waiting for changes from the author label Sep 2, 2024
@stanley31huang stanley31huang marked this pull request as draft September 4, 2024 02:54
fix unit tests
@stanley31huang stanley31huang marked this pull request as ready for review September 4, 2024 06:41
@stanley31huang
Copy link
Collaborator Author

For me it is very hard to understand if this is ok actually, I have a few nitpicks about the code but nothing major. The submissions you provide actually have 2 failed tests, so I'm not sure what is going on there.

Could this just randomly fail on some machines? Did you test on just two? Can you please test this on more?

I am sorry for the confusion on those failed test cases, it's trying to simulate some test failed scenario and make sure the network configuration could be restore. I have added more description for those submissions with failed tests, and attached a new submission without failed tests.

This is not ran during sru right?

I think so, it's not include in the SRU test plan as this is only include in the stress test plan.

(Oh and fantastic job with the context manager, I really like that!)

fix black issue
@Hook25 Hook25 merged commit d7d5c8c into main Sep 9, 2024
41 checks passed
@Hook25 Hook25 deleted the network_backup_iperf branch September 9, 2024 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-changes The review has been completed but the PR is waiting for changes from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants