-
Notifications
You must be signed in to change notification settings - Fork 67
Automate wake-on-LAN tests (New) #1819
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1819 +/- ##
==========================================
+ Coverage 52.33% 52.53% +0.20%
==========================================
Files 392 394 +2
Lines 42070 42268 +198
Branches 7796 7824 +28
==========================================
+ Hits 22018 22207 +189
- Misses 19275 19283 +8
- Partials 777 778 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@eugene-yujinwu |
@stanley31huang A link to the wol_server.py was added. Thank you. |
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
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'm sorry this review took so long! It looks like a sensible change, and the different examples you provided from C3 are really helpful to see it in action!
I left a few comments below, please check them out and let me know if you have questions.
providers/base/bin/wol_client.py
Outdated
req = urllib.request.Request(url, data=data_encoded, headers=headers) | ||
|
||
attempts = 0 | ||
while attempts < retry: |
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.
In checkbox_support
, we have a retry
helper that can be used to re-run a given function a certain number of times. See this example for how to use it, and check if it could be a good use case for here.
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.
Using a retry decorator appears more elegant, but the current approach is easier to understand and read, and it doesn’t require modifying the unit tests. If it be deemed necessary, I will make the corresponding changes.
In
checkbox_support
, we have aretry
helper that can be used to re-run a given function a certain number of times. See this example for how to use it, and check if it could be a good use case for here.
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.
For us, it's easier to maintain if new tests use the same retry mechanism, so please use it for your test. For the unit test, you can use mock_retry
which was implemented to make it easier to write unit tests for scripts that make use of @retry
. Please check test_eth_hotplugging.py for instance.
f0ffa6a
to
fa5a04e
Compare
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.
Thanks for the changes, I added a few more comments.
providers/base/bin/wol_client.py
Outdated
req = urllib.request.Request(url, data=data_encoded, headers=headers) | ||
|
||
attempts = 0 | ||
while attempts < retry: |
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.
For us, it's easier to maintain if new tests use the same retry mechanism, so please use it for your test. For the unit test, you can use mock_retry
which was implemented to make it easier to write unit tests for scripts that make use of @retry
. Please check test_eth_hotplugging.py for instance.
please refer to wake-on-LAN-automatic-tests.md for more details Co-authored-by: hanhsuan <[email protected]>
… checkbox. This is because it's not directly called by checkbox and introduces an unnecessary dependency on FastAPI.
…ues on some older python versions. Fix some issues per Stanley's review Add the related unit tests after code changes
Move inline function out of get_ip_mac per Stanley's comments
use more specific 4xx or 5xx status code from wol_server Co-authored-by: stanley31huang <[email protected]>
Check the return value when get the timestamp to prevent unexpected exception Update related unit tests
f0bb3ed
to
43ea23c
Compare
43ea23c
to
a63674d
Compare
@pieqq Thank you for the review. New commit was pushed. |
Description
This is a clone PR of #1686.
The 1686 was not reviewed by Cert team, and it was reverted and this PR was re-submitted for review.
Please refer more detailed info in the #1686.
Thank you.
We would like to automate the wake-on-LAN test instead of running manual tests for it.
Please refer to wake-on-LAN-automatic-tests.md in the commit for more details
Resolved issues
Make the Wake-on-LAN test from manual to automatic.
Documentation
https://github.com/canonical/checkbox/pull/1819/files#diff-cb60a2a6986f9c704b5a299f24f29bbfc64064adc94aec85d4fa75c167ccbb01
Tests
"[wol_S3_auto] Both success": https://certification.canonical.com/submissions/status/300209
"[wol_S3_auto] Cannot connect to the WOL server" : https://certification.canonical.com/submissions/status/300211
"[wol_S3_auto] One of the WOL disabled" : https://certification.canonical.com/submissions/status/300210
"[wol_auto_test] One of NIC has wake-on-LAN issue" : https://certification.canonical.com/submissions/status/300212
"[wake-on-Lan] test for one NIC": https://certification.canonical.com/hardware/202406-34088/submission/408713/