firmware/fwts_wakealarm.* and firmware/fwts_uefirtvariable.* not run

Bug #1502816 reported by Jerry Kao
14
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Checkbox Provider - Base
Fix Released
Critical
Po-Hsu Lin
OEM QA Checkbox additions
Fix Released
High
Pierre Equoy
Provider for Plainbox - Canonical Certification (Legacy)
Fix Released
Critical
Po-Hsu Lin

Bug Description

firmware/fwts are replaced by firmware/fwts_desktop_diagnosis and firmware/fwts_desktop_diagnosis_hwe. But firmware/fwts_wakealarm.* and firmware/fwts_uefirtvariable.* have dependency with firmware/fwts. After firmware/fwts was removed, blocker test cases, firmware/fwts_wakealarm.* and firmware/fwts_uefirtvariable.* do not run.

Related branches

Jerry Kao (jerry.kao)
tags: added: ce-qa-concern
Pierre Equoy (pieq)
Changed in plainbox-provider-checkbox:
status: New → Confirmed
importance: Undecided → Critical
Revision history for this message
Sylvain Pineau (sylvain-pineau) wrote :

fwts_uefirtvariable requires firmware/fwts to run first so a fix would be to restore the local job in the testplans.

Regarding firmware/fwts_wakealarm, this test is not even generated by the local job, but a similar job exists in power management:

power-management/fwts_wakealarm

Revision history for this message
Sylvain Pineau (sylvain-pineau) wrote :

Let's add back wakealarm in fwts_test QA_TESTS section. and make sure that we run the local job in the testplans.

Changed in plainbox-provider-checkbox:
status: Confirmed → Triaged
Revision history for this message
Po-Hsu Lin (cypressyew) wrote :
Changed in plainbox-provider-checkbox:
assignee: nobody → Po-Hsu Lin (cypressyew)
status: Triaged → In Progress
Revision history for this message
Po-Hsu Lin (cypressyew) wrote :

We do have uefirtvariable test in the fwts_test script of the QA section:

QA_TESTS = ['acpitests',
            'acpidump',
            'acpitables',
            'apicinstance',
            'aspm',
            'bios32',
            'dmicheck',
            'ebda',
            'mpcheck',
            'msr',
            'nx',
            'uefirtvariable',
            'version']

And for the fwts_wakealarm test, a quick fix will be just as Sylvain suggested in comment #2, add it into the QA_TESTS list. And we will be able to remove these two test cases from the test plan (as they will be tested with the fwts_desktop_diagnosis test).

So for now I would like to know do we need these test cases explicitly in the test plan, or can we just integrate them into the fwts_test (to me, I think it's safe to integrate them, as we already done this for the uefirtvariable for a while, and there is no other job depends on the fwts_wakealarm test, unless we're having these for some specific reason)

Changed in plainbox-provider-checkbox:
status: In Progress → Incomplete
Revision history for this message
Sylvain Pineau (sylvain-pineau) wrote :

jobs requiring the wakeup alarm to work do depend on power-management/fwts_wakealarm. so indeed there's no need to have fwts/wakealarm explicitly if already part of fwts_desktop_diagnosis.

Let's just remove them from the testplans. Needless to re introduce firmware/fwts if both wakealarm and uefirtvariable are run this way.

Thanks for your comment Po-Hsu.

Po-Hsu Lin (cypressyew)
Changed in plainbox-provider-checkbox:
status: Incomplete → In Progress
Revision history for this message
Po-Hsu Lin (cypressyew) wrote :

After more discussion with Sylvain, if we want to retain the blocker status of these two cases, we will need to add the generator back to make them work.

If we integrate these two test cases into the fwts_test script and make it to be tested with fwts_desktop_diagnosis test, it won't have an explicit pass / fail status for these two blockers.

I will add the generator back.

And I think this redundancy worth a new bug so it can be discussed again when we're making new test plan / coverage guide, or even changing the current test plan.

Revision history for this message
Po-Hsu Lin (cypressyew) wrote :

We need some info from the HWE team, for:
* why the uefirtvariable test is a blocker but other uefi related test cases are not?

And as Jerry mentioned on the IRC:
<JerryKao> spineau, PHLin it is not a good idea to run the test twice, once it fails, QA will file duplicate bugs for one issue.

It's better to figure out how to not duplicate them.

* uefirtvariable test: if we're gonna keep it explicitly in the test plan, we can move the test from QA_TESTS list to NON_CERT list. So it won't be triggered in the fwts_desktop_diagnosis nor the fwts_desktop_diagnosis_hwe test. And yet it's still visible with "fwts_test --list" command, so it can be generated with the local job.

* wakealarm test: we do have it in the fwts_test script, but I don't know why we have it in the "--all" flag, I guess we accidentally left it behind:
   elif args.all:
        tests.extend(['wakealarm', 'cpufreq', 'maxfreq'] + TESTS)

As stated in comment #1, to make the local job work, it needs to be relocated into one of the list (QA_TEST / NON_CERT_TESTS / HWE_TESTS), otherwise it won't be generated at all.

In the power-management/fwts_wakealarm job, it looks like it won't check the fail status, unless it's aborted. We just run it and get the log (correct me if I was wrong about the "-f" flag).
fwts_test -f aborted -t wakealarm -l $PLAINBOX_SESSION_SHARE/fwts-wakealarm.log

If we're going to integrate the power-management/fwts_wakealarm and firmware/fwts_wakealarm and retain the blocker pass / fail status (which means that we will check the return value of this test), those jobs that depend on this wakealarm test might be skipped if this one fails.

Changed in plainbox-provider-checkbox:
status: In Progress → Incomplete
Revision history for this message
Jerry Kao (jerry.kao) wrote :

<alexhung> JerryKao, uefirtvariable returns high failures, but wakealarm doesn't return high or critical
<JerryKao> alexhung, it is valuable info! so do you think if uefirtvariable returns high failures, it is a critical/blocker issue?
<alexhung> JerryKao, yes I think all UEFI runtime services should work flawless unless they aren't support.
<JerryKao> alexhung, anthonywong thanks to confirm
<alexhung> JerryKao, there are also other UEFI runtime services, but I will discuss with Ivan, and we will confirm whether they should be tested, too

Alex@HWE confirms that uefirtvariable and wakealarm should be blockers once it failed. So they should be separated from fwts_desktop_diagnosis.

Changed in plainbox-provider-checkbox:
status: Incomplete → Confirmed
Revision history for this message
Po-Hsu Lin (cypressyew) wrote :

Thanks for the information!
In that case, I will keep these two jobs in place and add the local job back.

And let's follow up other UEFI related tests in fwts_test later.

Changed in plainbox-provider-checkbox:
status: Confirmed → In Progress
Revision history for this message
Po-Hsu Lin (cypressyew) wrote :

Add the CDTS project, as the fwts local job name will be changed into:
generator_fwts
generator_fwts_logs

in the patch for Checkbox provider.

Changed in plainbox-provider-canonical-certification:
assignee: nobody → Po-Hsu Lin (cypressyew)
importance: Undecided → Critical
status: New → In Progress
Changed in cdts:
assignee: nobody → Po-Hsu Lin (cypressyew)
status: New → Incomplete
status: Incomplete → In Progress
importance: Undecided → High
Revision history for this message
Po-Hsu Lin (cypressyew) wrote :
tags: added: scripts
Po-Hsu Lin (cypressyew)
Changed in plainbox-provider-canonical-certification:
status: In Progress → Fix Committed
milestone: none → 0.19
Po-Hsu Lin (cypressyew)
Changed in plainbox-provider-checkbox:
status: In Progress → Fix Committed
milestone: none → 0.23
Revision history for this message
Pierre Equoy (pieq) wrote :

@Po-Hsu Lin: Is there anything required for the oem-plainbox-providers in order to comply with this fix? From your merge requests it looks like we need to replace `firmware/fwts` with `firmware/generator_.*`?

Revision history for this message
Po-Hsu Lin (cypressyew) wrote :

Yes!
The old firmware/fwts and firmware/fwts_log local jobs are renamed with a generator prefix,
so we just need to change the OEM provider, use "firmware/generator_.*" to replace these two.
Thanks

Po-Hsu Lin (cypressyew)
Changed in oem-qa-checkbox:
assignee: nobody → Pierre Equoy (pierre-equoy)
Pierre Equoy (pieq)
Changed in oem-qa-checkbox:
importance: Undecided → High
status: New → In Progress
Revision history for this message
Pierre Equoy (pieq) wrote :

Fix added in v0.25 of plainbox OEM providers.

Changed in oem-qa-checkbox:
status: In Progress → Fix Released
Changed in cdts:
status: In Progress → Fix Committed
Po-Hsu Lin (cypressyew)
Changed in cdts:
milestone: none → 1.12
Pierre Equoy (pieq)
Changed in plainbox-provider-checkbox:
status: Fix Committed → Fix Released
Changed in cdts:
status: Fix Committed → Fix Released
Changed in plainbox-provider-canonical-certification:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.