scripts/lib/robot.sh: Prevent running on dirty tree #946
Conversation
Signed-off-by: Filip Gołaś <filip.golas@3mdeb.com>
| if ! git diff --quiet; then | ||
| if [[ -z "${ALLOW_DIRTY}" ]]; then | ||
| echo "Git tree is dirty!" | ||
| echo "Commit your changes before running tests!" |
There was a problem hiding this comment.
| echo "Commit your changes before running tests!" | |
| echo "Commit and push your changes before running tests!" |
Having unknown revision from local commit would be worse than -dirty. Even though it isn't tested, having it printed may be enough. I also don't think that it needs to be tested, in case CI or some other script does a shallow checkout that may leave the repo in detached HEAD state.
There was a problem hiding this comment.
Do you really commit and push all of your code (not just test code) prior doing any test on it?
There was a problem hiding this comment.
Do you really commit and push all of your code (not just test code) prior doing any test on it?
I think that in the case of release tests, that should be the only possibility.
For any other scenario, like testing your code not the firmware - ALLOW_DIRTY
There was a problem hiding this comment.
Even though it isn't tested,
I think it might be possible to test it
There was a problem hiding this comment.
Let's then update to Krystian's suggestion and go ahead with this
There was a problem hiding this comment.
Added a check for pushing the changes to remote
❯ git add scripts/
❯ git commit -sS -m "scripts/lib/robot.sh: Check if local commit pushed to remote"
check for added large files..............................................Passed
check for merge conflicts................................................Passed
check for broken symlinks............................(no files to check)Skipped
detect private key.......................................................Passed
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
mixed line ending........................................................Passed
codespell................................................................Passed
markdownlint.........................................(no files to check)Skipped
markdownlint-fix.....................................(no files to check)Skipped
ShellCheck v0.10.0.......................................................Passed
isort (python).......................................(no files to check)Skipped
black................................................(no files to check)Skipped
robotidy.............................................(no files to check)Skipped
robocop..............................................(no files to check)Skipped
reuse lint-file..........................................................Passed
platform-configs.....................................(no files to check)Skipped
test-ids-check.......................................(no files to check)Skipped
test-ids-sort........................................(no files to check)Skipped
Check Platform Config Variables......................(no files to check)Skipped
Conform..................................................................Passed
[prevent-dirty-runs 25aa46de8fac] scripts/lib/robot.sh: Check if local commit pushed to remote
1 file changed, 16 insertions(+), 3 deletions(-)
❯ git push --force-with-lease
❯ ./scripts/run.sh util/test.robot
Checking if running the test from a reproducible revision.
If NECESSARY, set ALLOW_DIRTY to skip the check.
Local branch prevent-dirty-runs is ahead of origin/prevent-dirty-runs by 2 commits!
Push your changes before running any tests!
❯
There was a problem hiding this comment.
I don't think we can reasonably do much about changing the git history though. When someone does a rebase, all the commit IDs will change and GH will eventually remove those orphan commits from the history.
There was a problem hiding this comment.
But the test still starts despite warning, was that your goal?
NO_SETUP="true" FW_FILE=protectli_vp46xx_v1.2.0.rom DEVICE_IP=192.168.22 RTE_IP=192.168.10.203 CONFIG=protectli-vp4650 ./scripts/run.sh ./util/basic-platform-setup.robot
Checking if running the test from a reproducible revision.
If NECESSARY, set ALLOW_DIRTY to skip the check.
Logs will be saved at logs/protectli-vp4650/basic-platform-setup_2025_07_18_14_50_11
Watch "logs/protectli-vp4650/basic-platform-setup_2025_07_18_14_50_11/basic-platform-setup_debug.log" to monitor the progress of the test
==============================================================================
Basic-Platform-Setup
==============================================================================
BPS001.001 Power Control - PSU ON and serial output :: Verifies if... | FAIL |
Parent suite setup failed:
Error checking out asset 44. Response data: {'status': 'error', 'messages': 'That asset is not available for checkout!', 'payload': {'asset': '00044'}}
------------------------------------------------------------------------------
There was a problem hiding this comment.
Sorry, it does not. But in such case, If NECESSARY, set ALLOW_DIRTY to skip the check. should not be displayed if there is nothing to skip?
There was a problem hiding this comment.
After reflection, I would also try to explain the rationale behind this to the reader. E.g.:
echo "Please commit your changes before running tests to ensure reproducibility. (Set ALLOW_DIRTY=1 to override)"
Understanding the reason (e.g., ensuring reproducibility via clean git state) makes it more likely users will follow the rule properly, rather than bypassing it out of frustration.
|
@philipanda What's the origin story of this? I am afraid it would cause more workarounds and skipping usage of the run.sh Better place to put it would be regression.sh, not for every single test. |
OSFV dashboard logs the OSFV revision used for running the tests. It would be incredible to be able to actually reproduce the historic test results, but as of now nothing prevents the test to be run on dirty tree with arbitrary modifications to OSFV. I know that @krystian-hebel strongly insisted before on enforcing that, and I agree with him.
That's why I've already added a workaround - setting the
Eventually, yes. As of now I don't think |
Signed-off-by: Filip Gołaś <filip.golas@3mdeb.com>
philipanda
force-pushed
the
prevent-dirty-runs
branch
from
25aa46d to
97341b5
Compare
| echo "Checking if running the test from a reproducible revision." | ||
| echo "If NECESSARY, set ALLOW_DIRTY to skip the check." | ||
|
|
||
| if ! git diff --quiet || ! git diff --staged --quiet; then |
There was a problem hiding this comment.
Just a note that it does not include untracked files. Not sure if intended or not. Typically, we call a repo -dirty if there are new untracked files as well.
No description provided.