Skip to content

scripts/lib/robot.sh: Prevent running on dirty tree #946

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

scripts/lib/robot.sh: Prevent running on dirty tree #946

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31bf183
scripts/lib/robot.sh: Prevent running on dirty tree
philipanda Jul 18, 2025
97341b5
scripts/lib/robot.sh: Check if local commit pushed to remote
philipanda Jul 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions scripts/lib/robot.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,29 @@ execute_robot() {
installed_dut_option=""
fi

# Prevent executing tests on a dirty git tree

if [[ -z "${ALLOW_DIRTY}" ]]; then
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
Copy link
Contributor

@macpijan macpijan Jul 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

echo "Git tree is dirty!"
echo "Commit your changes before running tests!"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really commit and push all of your code (not just test code) prior doing any test on it?

Copy link
Contributor Author

@philipanda philipanda Jul 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though it isn't tested,

I think it might be possible to test it

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's then update to Krystian's suggestion and go ahead with this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check for pushing the changes to remote

25aa46d

❯ 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!
❯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@macpijan macpijan Jul 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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'}}
------------------------------------------------------------------------------

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

exit 1
fi

branch=$(git rev-parse --abbrev-ref HEAD)
git fetch -q
commits_ahead=$(git rev-list --left-right --count origin/$branch...$branch | awk '{print $2}')
if [[ "$commits_ahead" -gt 0 ]]; then
echo "Local branch $branch is ahead of origin/$branch by $commits_ahead commits!"
echo "Push your changes before running any tests!"
exit 1
fi
fi


# To save the logs from test modules into separate files robot is called
# multiple times.
#
Expand Down