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.

Sign up for GitHub

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

Jump to bottom

Update possible certification status values for Job units and modify its default (Breaking) #1204

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Update possible certification status values for Job units and modify its default (Breaking) #1204

wants to merge 11 commits into from

Conversation

pieqq
Copy link
Collaborator

@pieqq pieqq commented Apr 23, 2024

Description

Until now, a Job unit could have 4 possible values for certification status (unspecified, not-part-of-certification, blocker, non-blocker). This PR reduces the number to 2 (blocker and non-blocker), and set the default to non-blocker (instead of unspecified in the past).

Resolved issues

CER-2586

Documentation

  • Job unit reference page is updated
  • Examples making use of unspecified certification values are updated throughout the docs
  • Checkbox submission JSON schema updated

Tests

The disk-cert-automated nested part has a mix of default and cert-blockers.

Running checkbox-cli list-bootstrapped -f "{id} ({certification_status})\n" "com.canonical.certification::disk-cert-automated" allows to see the difference before and after the patch.

Before:

executable (unspecified)
disk/detect (blocker)
disk/stats_dm-1 (unspecified)
disk/read_performance_dm-1 (blocker)
disk/storage_device_dm-1 (blocker)
benchmarks/disk/hdparm-read_dm-1 (unspecified)
benchmarks/disk/hdparm-cache-read_dm-1 (unspecified)
disk/apste_support_on_nvme0 (unspecified)
disk/apste_support_on_nvme1 (unspecified)
manifest (unspecified)
disk/check-software-raid (unspecified)

After:

executable (non-blocker)
disk/detect (blocker)
disk/stats_dm-1 (non-blocker)
disk/read_performance_dm-1 (blocker)
disk/storage_device_dm-1 (blocker)
benchmarks/disk/hdparm-read_dm-1 (non-blocker)
benchmarks/disk/hdparm-cache-read_dm-1 (non-blocker)
disk/apste_support_on_nvme0 (non-blocker)
disk/apste_support_on_nvme1 (non-blocker)
manifest (non-blocker)
disk/check-software-raid (non-blocker)

I also ran this test plan before and after the patch and uploaded the results to C3.

Before:

image

After:

image

@pieqq
Set default Job certification status to "non-blocker"
b88f923 
Until now, the default was "unspecified". Set it to "non-blocker". In
addition, remove both "unspecified" and "non-part-of-certification"
statuses, since one is redundant and the other has never been used in
practice.

Fix CER-2586
@pieqq
Update default certification status for checkbox commands and exporters
91ee527
@pieqq
Update HTML exporter unit tests to match new default cert status value
80689d3 
Since there are only two possible certification statuses now, remove
tests that are not required anymore.
@pieqq
Update exporter templates to use the new default cert status value
b833fdb 
Until now, jobs with an "unspecified" certification status would not
show any certification status in the HTML report. Since "non-blocker"
becomes the default, the same behavior is applied. Therefore, only jobs
with a "blocker" certification status will specify their certification
status. "non-blocker" is assumed for all the others.
@pieqq
Update documentation
5f29d00
@pieqq
Update JSON submission schema
286f7a5
@codecov Codecov
Copy link

codecov bot commented Apr 23, 2024 •
edited

Codecov Report

Attention: Patch coverage is 88.88889% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 43.37%. Comparing base (84878c2) to head (0892f75).
Report is 7 commits behind head on main.

Files Patch % Lines
checkbox-ng/plainbox/impl/exporter/xlsx.py 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1204      +/-   ##
==========================================
+ Coverage   43.21%   43.37%   +0.16%     
==========================================
  Files         356      356              
  Lines       38662    38660       -2     
  Branches     6561     6561              
==========================================
+ Hits        16706    16768      +62     
+ Misses      21293    21213      -80     
- Partials      663      679      +16
Flag Coverage Δ
checkbox-ng 67.98% <88.88%> (+0.43%) ⬆️

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.

@pieqq pieqq force-pushed the CER-2586-two-cert-status-only branch from 3694b31 to 427414a Compare April 23, 2024 08:46
@pieqq pieqq mentioned this pull request Apr 24, 2024
Open
Hook25
Hook25 approved these changes Apr 29, 2024
View reviewed changes
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.

+1 on the change, less random statusses for jobs is better! A few questions, the most important one is: this doesn't change anything in the remote codebase (as far as I can see). Is this change propagated there somehow I can't understand right now? If so, how?

Minor other questions below.

checkbox-ng/checkbox_ng/launcher/subcommands.py Show resolved Hide resolved
checkbox-ng/plainbox/impl/exporter/test_html.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

The html end to end testing is pretty weird and not a unit test for sure, but it is not 100% pointless. Why did you remove them instead of updating them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because we went from 4 kinds of cert status to 2, so the test matrix is much smaller and some tests were redundant, so I removed them, which included removing the associated generated sample HTML files.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok I was kind of asking why not re-sampling the "golden" result but I guess that if the coverage is not impacted by the removal fire on!

checkbox-ng/plainbox/test-data/html-exporter/with_both_certification_status.html
Copy link
Collaborator

Choose a reason for hiding this comment

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

related to removal above, why?

checkbox-ng/plainbox/test-data/html-exporter/with_certification_non_blocker.html
Copy link
Collaborator

Choose a reason for hiding this comment

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

related to removal above, why?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hook25 Hook25 Hook25 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pieqq @Hook25