Replace `check_notebook.sh` with appropriate Python script (New) by motjuste · Pull Request #1725 · canonical/checkbox

motjuste · 2025-02-14T17:00:12Z

Description

This is the first piece of the original PR #1724 to enable easier review. The main changes in this PR include (after also applying the requests from review):

Moved the in-line Python scripts that need to be run in the pods from check_notebook.sh to individual scripts in the data directory.
Added a check_notebook.py that accepts a notebook's name and the name of the script to run in its pod, finds the appropriate pod and runs the script in that pod.
Replaced existing uses of check_notebook.sh in the jobs to analogous uses of check_notebook.py.
Removed the now obsolete check_notebook.sh.
Added unit-tests for check_notebook.py, and the ability to run them using tox.
Added a new GitHub workflow to run these unit-tests for any PRs to this contrib area.

Resolved issues

Part of CHECKBOX-1693

Documentation

No changes to Checkbox documentation.

Tests

Please see comment from codecov for the coverage report for unit-tests added in this PR. This branch was also verified to run in Testflinger.

check_notebook.py is going to replace check_notebook.sh, and it uses some utilities from common.py (which later Python scripts will also use).

those Python files were scripts that were being submitted to the notebook pods. These scripts are now in-line in the check_notebook.py

it is required that dss is purged before the integration tests are run, hence a tenmporary job for it has been added that does not do any explicit verification that the purge was successful (except for exit code perhaps). This tmp dss/purge job will be replaced by a better impl in #1725

fernando79513

Thanks for the PR and sorry for taking so long to review it.
I have added some comments here and there, but the main concern is that this PR is much more complex that it needs to be.
There are too many helper functions that could be simplified since they are not adding anything important to the test.
If something is not clear enough, please ping me so we can discuss it.

contrib/checkbox-dss-validation/checkbox-provider-dss/bin/check_notebook.py

.github/workflows/tox-contrib-provider-dss.yaml

contrib/checkbox-dss-validation/checkbox-provider-dss/bin/check_notebook.py

contrib/checkbox-dss-validation/checkbox-provider-dss/bin/common.py

Co-authored-by: Fernando Bravo <39527354+fernando79513@users.noreply.github.com>

codecov · 2025-02-19T13:16:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.47%. Comparing base (0a49970) to head (85f75f1).
Report is 143 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1725      +/-   ##
==========================================
+ Coverage   49.23%   49.47%   +0.23%     
==========================================
  Files         373      376       +3     
  Lines       40400    40591     +191     
  Branches     6822     6822              
==========================================
+ Hits        19890    20081     +191     
  Misses      19784    19784              
  Partials      726      726

Flag	Coverage Δ
provider-dss	`∅ <100.00%> (?)`

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

motjuste

I have replied to all the comments, accepted one change to add coverage reporting, and will work on updating the Copyright banners on the rest. @fernando79513 please have a look at my clarifications and questions for the rest of the comments.

motjuste · 2025-02-19T14:10:42Z

With the recent commits, I have integrated more suggestions and resolved relevant conversations from my side. For the rest, I have provided my justifications.

contrib/checkbox-dss-validation/checkbox-provider-dss/bin/check_notebook.py

motjuste · 2025-02-20T11:06:36Z

Thank you for the recent comments. I am working on incorporating your suggestions and will update.

Let's not chicken out

motjuste · 2025-02-20T15:31:29Z

I am working on shipping the scripts as part of ./data, but the rest of the feedback has been incorporated. @fernando79513, please have another look.

contrib/checkbox-dss-validation/checkbox-provider-dss/bin/check_notebook.py

fernando79513 · 2025-02-24T11:59:00Z

Helo @motjuste!
Thanks for all the work here! I've just added some comments to the PR.

Running these scripts never timeout in practice, and shouldn't based on where we are going. Nevertheless, timeout can be set by the caller, e.g. like `timout 5m check_notebook.py ...`

The script and associated jobs are BROKEN in this commit

motjuste · 2025-02-25T12:42:49Z

Hey @fernando79513, I believe I have now covered all your requests from the review (thanks a LOT for the very careful and useful comments!). I have updated the PR's description now to reflect what will be merged.

fernando79513

Thank you for all the changes!
Really good job here.

- Moved the in-line Python scripts that need to be run in the pods from check_notebook.sh to individual scripts in the data directory. - Added a check_notebook.py that accepts a notebook's name and the name of the script to run in its pod, finds the appropriate pod and runs the script in that pod. - Replaced existing uses of check_notebook.sh in the jobs to analogous uses of check_notebook.py. - Removed the now obsolete check_notebook.sh. - Added unit-tests for check_notebook.py, and the ability to run them using tox. - Added a new GitHub workflow to run these unit-tests for any PRs to this contrib area.

motjuste added 5 commits February 14, 2025 22:10

add check_notebook.py and common.py

260dab8

check_notebook.py is going to replace check_notebook.sh, and it uses some utilities from common.py (which later Python scripts will also use).

replaced calls to check_notebook.sh with app. .py

b93dec0

removed check_notebook.sh and associated files

8ee69db

those Python files were scripts that were being submitted to the notebook pods. These scripts are now in-line in the check_notebook.py

added tox file to run unittests and documented it

1349eaa

added GH workflow to run dss-provider unittests

63e9186

motjuste requested a review from a team as a code owner February 14, 2025 17:00

motjuste mentioned this pull request Feb 14, 2025

Replaced DSS shell scripts with Python and added running DSS integration tests (New) #1724

Closed

motjuste requested review from pieqq and removed request for a team February 14, 2025 17:02

This was referenced Feb 14, 2025

Replace check_dss.sh with an appropriate Python script (New) #1726

Closed

Replace check_cuda.sh with an appropriate Python script (New) #1727

Closed

Replace check_intel.sh with an appropriate Python script (New) #1728

Closed

This was referenced Feb 14, 2025

Add dss integration tests and update Testflinger queues in GH workflow (New) #1729

Closed

Add DSS reboot testing (New) #1730

Closed

fernando79513 self-assigned this Feb 17, 2025

fernando79513 reviewed Feb 19, 2025

View reviewed changes

Add coverage report to the testing pipeline

5a56cf3

Co-authored-by: Fernando Bravo <39527354+fernando79513@users.noreply.github.com>

motjuste commented Feb 19, 2025

View reviewed changes

motjuste added 5 commits February 19, 2025 18:59

Fix copyright information in Python scripts

0fe22ca

Disable coverage of test scripts

d76f283

Disable coverage for __main__ block

3c9e203

Replace using assert with raising AssertionError

b070157

Simplify assertions in scripts to run in notebooks

213b46e

fernando79513 reviewed Feb 19, 2025

View reviewed changes

contrib/checkbox-dss-validation/checkbox-provider-dss/bin/check_notebook.py Outdated Show resolved Hide resolved

motjuste added 2 commits February 20, 2025 16:48

Remove overridden run_command

05b1aaa

Rename function to get_notebook_pod

4465c6a

motjuste added 11 commits February 20, 2025 17:16

Use sp.check_output directly in get_notebook_pod

bd61b90

Rename func to run_script_in_notebook

c968efa

Remove using SUCCESS_MARKER in scripts

505c50f

Rename test for running script in notebook

991df81

use sp.check_call directly in running scripts

596b9c5

Remove unused run_script_in_pod & reformat

8aeb055

Use jsonpath to get only pod names

61d7eaa

Remove unused run_command from common

0994c6a

Add missing tests for cmd parser; enable coverage

5e81428

Cleanup and enable coverage of test scripts too

65a8f9f

Let's not chicken out

Fix formatting

4b69fd0

motjuste changed the title ~~Replace check_notebook.sh with and appropriate Python script (New)~~ Replace check_notebook.sh with appropriate Python script (New) Feb 20, 2025

fernando79513 reviewed Feb 24, 2025

View reviewed changes

contrib/checkbox-dss-validation/checkbox-provider-dss/bin/check_notebook.py Outdated Show resolved Hide resolved

motjuste added 5 commits February 24, 2025 22:38

Add scripts to run in notebooks as provider data

bc95e8d

Remove specifying timeout in running scripts in nb

0bb4d74

Running these scripts never timeout in practice, and shouldn't based on where we are going. Nevertheless, timeout can be set by the caller, e.g. like `timout 5m check_notebook.py ...`

Remove soon to be obsolete impl of checks in nb

143d2e4

The script and associated jobs are BROKEN in this commit

Add new arg parser and main for checking notebooks

7e66351

Update jobs to new commands for check_notebook

85f75f1

motjuste mentioned this pull request Feb 25, 2025

Replace check_cuda.sh with simple in-line commands (New) #1750

Merged

fernando79513 approved these changes Feb 26, 2025

View reviewed changes

motjuste merged commit 1721cc3 into main Feb 26, 2025
19 checks passed

motjuste deleted the CHECKBOX-1693-replace-check_notebook-sh-with-py branch February 26, 2025 12:27

Conversation

motjuste commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Resolved issues

Documentation

Tests

Uh oh!

fernando79513 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

motjuste left a comment

Choose a reason for hiding this comment

Uh oh!

motjuste commented Feb 19, 2025

Uh oh!

Uh oh!

motjuste commented Feb 20, 2025

Uh oh!

motjuste commented Feb 20, 2025

Uh oh!

Uh oh!

fernando79513 commented Feb 24, 2025

Uh oh!

motjuste commented Feb 25, 2025

Uh oh!

fernando79513 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

motjuste commented Feb 14, 2025 •

edited

Loading

codecov bot commented Feb 19, 2025 •

edited

Loading