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

Open
wants to merge 17 commits into
base: main
Choose a base branch
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

All modified and coverable lines are covered by tests ✅

Project coverage is 47.60%. Comparing base (2d42b1e) to head (8ec106c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1309      +/-   ##
==========================================
+ Coverage   47.55%   47.60%   +0.05%     
==========================================
  Files         369      369              
  Lines       39585    39589       +4     
  Branches     6685     6687       +2     
==========================================
+ Hits        18824    18846      +22     
+ Misses      20050    20032      -18     
  Partials      711      711              
Flag Coverage Δ
provider-base 24.18% <100.00%> (+0.13%) ⬆️

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 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
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.

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