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

Use the retry decorator in WiFi tests (Bugfix) #1488

Merged
merged 17 commits into from
Sep 24, 2024

Conversation

pieqq
Copy link
Collaborator

@pieqq pieqq commented Sep 19, 2024

Description

The main raisons d'être of this PR are:

  • Use the newly developed retry decorator available in checkbox-support to implement a retry mechanism in the wifi_nmcli_test.py script. This is done to prevent an often seen failure in the lab where the usual answer is to rerun the whole test plan. Now, the WiFi test retries up to 5 times, adding a delay between each retry.
  • Refactor the script so that functions raise exceptions instead of returning/handling return codes. This is required for the retry decorator to work properly, and is generally speaking a good practice in Python.

Resolved issues

CHECKBOX-1543

Documentation

Nothing is modified in terms of Checkbox jobs or external-facing documentation.

Tests

  • Unit tests pass on Python 3.5+
  • The script was tested successfully on my laptop

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 91.07143% with 5 lines in your changes missing coverage. Please review.

Project coverage is 47.50%. Comparing base (104c931) to head (e41f30d).
Report is 118 commits behind head on main.

Files with missing lines Patch % Lines
providers/base/bin/wifi_nmcli_test.py 90.74% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1488      +/-   ##
==========================================
+ Coverage   47.48%   47.50%   +0.01%     
==========================================
  Files         369      369              
  Lines       39586    39549      -37     
  Branches     6685     6678       -7     
==========================================
- Hits        18798    18788      -10     
+ Misses      20077    20050      -27     
  Partials      711      711              
Flag Coverage Δ
checkbox-support 60.49% <100.00%> (ø)
provider-base 24.02% <90.74%> (-0.01%) ⬇️

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.

This helps investigating issues when more than one functions are using
the retry decorator.
Both function are doing exactly the same thing, the only difference
being the command used.

Create a connection() function, and call it from open_connection() and
secured_connection().

Compared to the previous implementation, the connection() function is
more linear and does not rely on boolean return codes to decide what to
do. Instead, it's trying to do things and expect exceptions to be raised
in case of problem (this is why it's using subprocess.run(...,
check=True) for instance)
Parts of the wifi_nmcli_test.py script was implementing a basic retry
mechanism. Remove this to use the retry decorator instead.

In addition, raise exceptions rather than trying to catching them, or
using boolean return codes.
This is to avoid waiting for the actual retry decorator implementation.
@pieqq pieqq force-pushed the 1543-retry-decorator-wifi-tests branch from 1519956 to 3f52db9 Compare September 20, 2024 09:46
@Hook25
Copy link
Collaborator

Hook25 commented Sep 20, 2024

Testing this on 3 devices in the lab (focal, jammy, noble): https://github.com/canonical/checkbox/actions/runs/10958861773

For reference, here is the command:

gh workflow run dispatch_lab_job.yaml --ref 1543-retry-decorator-wifi-tests -f 'matrix_to_create=[{"data_source":"distro: desktop-24-04-uefi","queue":"201912-27623","match":".*wireless.*","test_plan":"com.canonical.certification::sru"},{"data_source":"distro: desktop-22-04-2-uefi","queue":"202002-27717","match":".*wireless.*","test_plan":"com.canonical.certification::sru"},{"data_source":"distro: desktop-20-04-1-uefi","queue":"201904-26941","match":".*wireless.*","test_plan":"com.canonical.certification::sru"}]'

Hook25
Hook25 previously approved these changes Sep 23, 2024
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.

LGTM +1, consider a few small changes here and there

providers/base/bin/wifi_nmcli_test.py Outdated Show resolved Hide resolved
providers/base/bin/wifi_nmcli_test.py Outdated Show resolved Hide resolved
providers/base/bin/wifi_nmcli_test.py Outdated Show resolved Hide resolved
providers/base/bin/wifi_nmcli_test.py Outdated Show resolved Hide resolved
providers/base/bin/wifi_nmcli_test.py Outdated Show resolved Hide resolved
providers/base/bin/wifi_nmcli_test.py Outdated Show resolved Hide resolved
providers/base/tests/test_wifi_nmcli_test.py Outdated Show resolved Hide resolved
@Hook25 Hook25 merged commit 0952eb2 into main Sep 24, 2024
45 checks passed
@Hook25 Hook25 deleted the 1543-retry-decorator-wifi-tests branch September 24, 2024 08:50
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.

2 participants