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

Extend the wwan connection tests to support multiple cycle connection tests (New) #1309

Closed
wants to merge 32 commits into from

Conversation

stanley31huang
Copy link
Collaborator

@stanley31huang stanley31huang commented Jun 24, 2024

Description

Extend the wwan connection tests to support multiple cycle connection tests, we found an issue that the LTE connection could not be established at second round.

As discussed internally with the QA team, we have all agreed that the default number of test runs for WWAN connections tests is 2.

Resolved issues

Here is the support ticket: 00379830.

Our SWE has identified this issue is related to the modem firmware and modem-manager snap, and they are trying to add a patch to modem-manager snap as a workaround.

Documentation

N/A

Tests

https://certification.canonical.com/hardware/202302-31253/submission/386810/test-results/

Copy link

codecov bot commented Jun 24, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 14 lines in your changes missing coverage. Please review.

Project coverage is 47.75%. Comparing base (2d42b1e) to head (cf8e538).
Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
providers/base/bin/reboot_check_test.py 92.15% 8 Missing and 4 partials ⚠️
checkbox-ng/checkbox_ng/launcher/subcommands.py 88.88% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1309      +/-   ##
==========================================
+ Coverage   47.55%   47.75%   +0.19%     
==========================================
  Files         369      370       +1     
  Lines       39585    39739     +154     
  Branches     6685     6622      -63     
==========================================
+ Hits        18824    18976     +152     
- Misses      20050    20058       +8     
+ Partials      711      705       -6     
Flag Coverage Δ
checkbox-ng 68.09% <88.88%> (-0.04%) ⬇️
provider-base 24.78% <93.25%> (+0.73%) ⬆️
provider-sru 97.97% <ø> (ø)

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.

Copy link
Collaborator

@pieqq pieqq left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! This is a good idea to try to connect WWAN multiple times, this will for sure help with regression testing.

A few things:

Default number of iterations

The default number of iterations is something that should be discussed with other QA teams, especially PC QA. With your proposal, if the WWAN_CONNECTION_TEST_COUNT environment variable is not set, there will only be one connection test. Maybe PC QA and other IoT QA projects would benefit from having several connection attempts?

What if connection fails at first attempt?

Should we still try X times, adding X failures to the test report?

Splitting connection jobs Vs. connecting multiple times in the same job

Is it important to have multiple jobs (resulting in several lines in the test report), or could the number of iterations provided to the wwan_tests.py script, and the connection would be attempted X times instead?

Code duplication

I think you could combine the new wwan_resource_with_iterations.py with the existing wwan_tests.py. The latter already provides the functionality to print out the resource; you could just add the --iterations argument to the wwan_tests.py script and use the same mechanism you developed in wwan_resource_with_iterations.py (repeat the resource info for each iteration).

By doing this, you

  • prevent code duplication
  • reduce the number of scripts to maintain
  • reduce the number of additional tests

Since you only want the connection test to be run multiple times, and not the other tests based on wwan_resource, an easy fix is to add

template-filter: wwan_resource.iteration == '1'

to these jobs. They will only be run for the first iteration.

I've tried it locally and it seems to work; here are the modifications I made to the WWAN jobs using wwan_resource:

diff --git a/providers/base/units/wwan/jobs.pxu b/providers/base/units/wwan/jobs.pxu
index 99cf4fb45..07d9fe901 100644
--- a/providers/base/units/wwan/jobs.pxu
+++ b/providers/base/units/wwan/jobs.pxu
@@ -27,7 +27,7 @@ requires:
   snap.name == 'modem-manager' or package.name == 'modemmanager'
 
 unit: template
-template-resource: wwan_resource_with_iteration
+template-resource: wwan_resource
 template-unit: job
 id: wwan/gsm-connection-{manufacturer}-{model}-{hw_id}-cycle{iteration}-auto
 template-id: wwan/gsm-connection-manufacturer-model-hw_id-auto
@@ -59,6 +59,7 @@ requires:
 unit: template
 template-resource: wwan_resource
 template-unit: job
+template-filter: wwan_resource.iteration == '1'
 id: wwan/check-sim-present-{manufacturer}-{model}-{hw_id}-auto
 template-id: wwan/check-sim-present-manufacturer-model-hw_id-auto
 _summary:  Check if a SIM card is present in a slot connected to the modem
@@ -79,6 +80,7 @@ requires:
 unit: template
 template-resource: wwan_resource
 template-unit: job
+template-filter: wwan_resource.iteration == '1'
 id: wwan/verify-sim-info-{manufacturer}-{model}-{hw_id}
 template-id: wwan/verify-sim-info-manufacturer-model-hw_id
 depends: wwan/check-sim-present-{manufacturer}-{model}-{hw_id}-auto

providers/base/units/wwan/resource.pxu Outdated Show resolved Hide resolved
providers/base/units/wwan/resource.pxu Outdated Show resolved Hide resolved
providers/base/units/wwan/jobs.pxu Outdated Show resolved Hide resolved
providers/base/units/wwan/jobs.pxu Outdated Show resolved Hide resolved
@stanley31huang
Copy link
Collaborator Author

@pieqq
Thanks for reviewing this PR and the suggestions. I like the idea to integrate the iteration into the wwan_resource job, to avoid the code duplication.

For the connections fail at the first attempt, I think it still worth to launch the rest of the connection tests, because of we could understand the failure rate is 100% or not. But it would be better to add a dependency in this job which is make sure the SIM card has been detected.

For the Splitting connection jobs Vs. connecting multiple times in the same job, I personally prefer the way to splitting connections jobs, because of it's easily to know the overall test results, no need to go through the details from the output of job.

@stanley31huang stanley31huang marked this pull request as draft July 9, 2024 00:49
@fernando79513 fernando79513 added the waiting-for-changes The review has been completed but the PR is waiting for changes from the author label Jul 16, 2024
@stanley31huang
Copy link
Collaborator Author

Converted this PR to draft, due to I am going to re-base this branch after another PR has been merged.
#1323

@stanley31huang stanley31huang marked this pull request as ready for review August 6, 2024 06:24
@stanley31huang stanley31huang marked this pull request as draft August 6, 2024 09:41
@stanley31huang stanley31huang marked this pull request as ready for review August 15, 2024 02:16
stanley31huang and others added 16 commits September 27, 2024 16:25
allow the current wwan connection test to support multiple cycle
connection tests
Modified the resource scripts and add unit tests
update test job
update the test case and scripts
apply context manager as setup and teardown functions during 3g
connection test
allow the current wwan connection test to support multiple cycle
connection tests
Modified the resource scripts and add unit tests
Co-authored-by: Pierre Equoy <pierre.equoy@canonical.com>
Co-authored-by: Pierre Equoy <pierre.equoy@canonical.com>
Co-authored-by: Pierre Equoy <pierre.equoy@canonical.com>
remove unnecessary files
update wwan_resource jobs
fix unittest
fix conflict in wwan test plan
fix bug
fix conflict
@pieqq
Copy link
Collaborator

pieqq commented Sep 27, 2024

Following today's Office Hours meeting, we discussed with QA and Checkbox team regarding this PR.

If the goal is "just" to run the jobs twice, then maybe we could just use a sibling, and add that new job to the test plan.

Hook25 and others added 9 commits September 27, 2024 15:19
* Also print manifest to expand

* Document and partially de-functionalize code

* Add tests for the new export units

* Also test text exporter for manifest exporting

* Check null tempalte-id
Correct the camera job cmd with correct camera tests
Fix snap refresh verification job dependency

When implementing changes for the gadget/snapd/kernel snap refresh[1],
one of the job dependencies was not updated properly.

The `snapd/reboot-after-snap-refresh-{type}-{name}-to-stable-rev` job
does not exist anymore, so the
`snapd/snap-verify-after-refresh-{type}-{name}-to-stable-rev` should
depend on the refresh job itself (which was modified in the
aforementioned PR to do the reboot itself)

[1]: #1124
* Add SIM card manifest entry

* Add SIM card requirement on WWAN jobs needing it

* Align `requires` and `depends` fields in WWAN jobs

Use 2 spaces instead of 4 to match the rest of the file.
…#1507)

Includes libasound2-dev as a Dependency in Build Instructions - Issue#1506
* Refactor workflow into action

* Fix typo and set working directory

* Provide secrets via additioanl launcher

* Use launcher from the correct location

* Print override to screen

* Propagate the checkbox_revision from the action input

* Partially support noprovision

* Allow degraded wait_for_ssh

* Call the clean_machine script in the workflow

* Also set and refresh zapper snap

* Documented new parameter and reset default value

* Disable failfast

* Shallow fetch the Checkbox repo

* Try to change the path in an action aware way

* Fix typo

Co-authored-by: Paolo Gentili <paolo.gentili@canonical.com>

* Document parameters

* Use the new git_get scriplet

* Check the copy path before copying

* Try to move the scriplet to /bin

* Use the new name of the script

* Now git_get_shallow is in main

* Change the action target branch to the dispatch action

---------

Co-authored-by: Paolo Gentili <paolo.gentili@canonical.com>
Use wwan-automated nested part in SRU test plan

Replace mobilebroadband-cert-automated with wwan-automated nested part,
as the wwan-automated test plan includes better coverage for
WWAN-related tests.

Add after-suspend-wwan-automated counterpart to make sure there are no
issues with WWAN after resuming from suspend.

This is a follow-up to work done by QA in the certification-client
desktop test plans (see #821)

Fix CER-2738
* feat: initial migration

* feat: created a wrapper for subprocess.run()

* fix: incorrect main function logic

* style: add more error messages

* fix: no display connectio n-> skipped

* refactor: move device comparison tests to a class

* test: unit tests for reboot_check_test

* fix: remove any direct calls to subprocess.run

* test: add a test for the all-flags-enabled case

* style: reorganize declarations

* fix: missing .value when using enums

* refactor: separate tests into more classes

* refactor: add clenup methods

* refactor: group tests into classes

* fix: update tests to reflect changes in reboot_check_test

* style: formatting, comments

* fix: extra -g flag, add comments

* fix: remove get_display_id due to too many edge cases

* fix: extra wrapper for non-existent commands

* style: fit everything within 80 char limit

* test: update unit tests to reflect new method names

* fix: assume display variable

* test: update tests to reflect removal of get_display_id

* fix: missing conditionals on certain logs

* fix: let unity support infer environment variable

* refactor: revert changes in boot.pxu to put in a separate PR

* fix: flake8 linter errors

* fix: increase test coverage

* fix: combine dpkg check conditions

* update: use the new desktop env detector

* test: increase coverage

* style: formatting

* fix: remove subprocess.run wrapper

* fix: use class attributes instead of enum to avoid writing .value

* fix: reflect changes in unit tests

* style: formatting
@stanley31huang stanley31huang marked this pull request as draft October 4, 2024 06:36
tomli380576 and others added 6 commits October 4, 2024 09:39
feat: update boot.pxu to reflect changes in reboot_check_test.py
* Howto use match

* Include it in the howto in the index

Minor: emph lines with changes

* Misspelled word

* Typos and suggestions

Co-authored-by: Pierre Equoy <pierre.equoy@canonical.com>

* Re-ordered sections

---------

Co-authored-by: Pierre Equoy <pierre.equoy@canonical.com>
apply siblings flag for wwan reconnection test
@stanley31huang
Copy link
Collaborator Author

close this one, due to the changes are bit messy. Use #1534 instead.

@stanley31huang stanley31huang deleted the wwan-multiple-test branch October 8, 2024 08:08
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.

7 participants