Specify fwts data directory for snap checkbox (Bugfix)#2323
Specify fwts data directory for snap checkbox (Bugfix)#2323tomli380576 wants to merge 6 commits intomainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2323 +/- ##
==========================================
+ Coverage 55.24% 55.32% +0.07%
==========================================
Files 413 413
Lines 44448 44618 +170
Branches 8194 8247 +53
==========================================
+ Hits 24556 24683 +127
- Misses 19032 19073 +41
- Partials 860 862 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes fwts execution inside snap-based checkbox by ensuring fwts reads its JSON data files from the checkbox runtime, and adds regression coverage for the new behavior.
Changes:
- Added
get_fwts_base_cmd()to select the correctfwtsinvocation (snap vs deb). - Updated fwts command construction in
main()to use the new base command. - Added unit tests covering snap/deb environment command selection and error behavior.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| checkbox-support/checkbox_support/scripts/fwts_test.py | Introduces snap-aware fwts base command and uses it when building fwts invocations. |
| checkbox-support/checkbox_support/scripts/tests/test_fwts_test.py | Adds unit tests validating base command selection and snap-directory error handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "We are in a snap environment, " | ||
| + "but '{}' ".format(fwts_json_data_dir) | ||
| + "doesn't exist" |
There was a problem hiding this comment.
The SystemExit message is a bit hard to read due to string concatenation and the extra space inside "but '{}' ". Consider simplifying into a single formatted string and making it more actionable (e.g., explicitly stating the expected directory and hinting that the snap/checkbox-runtime may be corrupted or missing the fwts data files). This will also make tests/assertions cleaner.
| "We are in a snap environment, " | |
| + "but '{}' ".format(fwts_json_data_dir) | |
| + "doesn't exist" | |
| "FWTS JSON data directory '{}' does not exist under the " | |
| "CHECKBOX_RUNTIME snap directory. The snap/checkbox-runtime " | |
| "may be corrupted or missing the FWTS data files.".format( | |
| fwts_json_data_dir | |
| ) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
(I accidentally broke #2252 during rebase xD this PR is a copy of that based on the current main)
Description
This PR fixes #1997 by specifying the
-joption in fwts when we detect that we are in a snap environment.Resolved issues
fwts klogresult #1997Documentation
Basically we are telling fwts to read the directory in checkbox runtime instead of whatever
/usr/share/fwtsis inside a snap.Tests
Original unit tests
Real hardware with snaps built here:
The tests failed from real fwts errors, but fwts was correctly invoked with
-j