Skip to content

Conversation

@Lmh-java
Copy link
Contributor

@Lmh-java Lmh-java commented Mar 15, 2025

Description

Add a python script that dry-runs some of our common CLI commands (as some sort of E2E integration test)

Testing Done

Resolved Issues

Length Justification and Key Files to Review

Review Checklist

It is the reviewers responsibility to also make sure every item here has been covered

  • Function & Class comments: All function definitions (usually in the .h file) should have a javadoc style comment at the start of them. For examples, see the functions defined in thunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.
  • Remove all commented out code
  • Remove extra print statements: for example, those just used for testing
  • Resolve all TODO's: All TODO (or similar) statements should either be completed or associated with a github issue

@github-actions
Copy link
Contributor

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale Inactive pull requests label Apr 15, 2025
@itsarune itsarune removed the Stale Inactive pull requests label Apr 16, 2025
common-commands:
strategy:
# TODO: set fail-fast to true in production.
Copy link
Contributor

Choose a reason for hiding this comment

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

remember to remove TODO before merging

- ../scripts/safe_run.sh bazel run --run_under="xvfb-run" //software/thunderscope:thunderscope_main -- --enable_autoref --ci_mode && ../scripts/safe_run.sh bazel run --run_under="xvfb-run" //software/thunderscope:thunderscope_main -- blue_log="$(find /tmp/tbots/blue -maxdepth 1 -type d -name 'proto_*' -printf '/tmp/tbots/blue/%f\n' 2>/dev/null | head -n1)"

name: Sanity Check on Common Commands
runs-on: ubuntu-20.04
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use the ubuntu-22.04 runner

commands:
- ../scripts/safe_run.sh bazel run --run_under="xvfb-run" //software/thunderscope:thunderscope_main -- --run_blue --run_diagnostics --interface lo --keyboard_estop --ci_mode
- ../scripts/safe_run.sh bazel run --run_under="xvfb-run" //software/ai/hl/stp/tactic/goalie:goalie_tactic_test -- --enable_thunderscope
- ../scripts/safe_run.sh bazel run --run_under="xvfb-run" //software/thunderscope:thunderscope_main -- --enable_autoref --ci_mode && ../scripts/safe_run.sh bazel run --run_under="xvfb-run" //software/thunderscope:thunderscope_main -- blue_log="$(find /tmp/tbots/blue -maxdepth 1 -type d -name 'proto_*' -printf '/tmp/tbots/blue/%f\n' 2>/dev/null | head -n1)"
Copy link
Contributor

Choose a reason for hiding this comment

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

what does the second half of this command do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay, I am finally done work.

../scripts/safe_run.sh bazel run --run_under="xvfb-run" //software/thunderscope:thunderscope_main -- --enable_autoref --ci_mode This first half runs thunderscope once

This part tries to find the previous replay log and display the logs:
../scripts/safe_run.sh bazel run --run_under="xvfb-run" //software/thunderscope:thunderscope_main -- blue_log="$(find /tmp/tbots/blue -maxde4pth 1 -type d -name 'proto_*' -printf '/tmp/tbots/blue/%f\n' 2>/dev/null | head -n1)

This is not ideal, I am trying to think of a way to handle this. Maybe I should allow a new argument for our thunderscope, so we can specify the location for the replay log to save, so that we don't need to lookup for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah being able to specify the runtime dir + folder name would be ideal

@@ -0,0 +1,52 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

safe_run.sh is sort of a weird name that doesn't immediately tell you what this file does

But tbh I don't know a better name... maybe ci_runner.sh is better. At least it tells you that it's only relevant for CI.

@@ -0,0 +1,52 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a high-level description of this script at the top of this file.

@@ -0,0 +1,52 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

If you re-write this script with python, you can use subprocess and get stdout and stderr logs. Then, you can print the error messages. Because right now, there isn't any feedback for the developers and it will be confusing when this CI job suddenly fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, sounds good!
I was trying to use 2> and > to redirect stdout and stderr, but it didn't seem to work.

Copy link
Contributor

@itsarune itsarune left a comment

Choose a reason for hiding this comment

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

looks awesome! left some interim feedback for you

Comment on lines +94 to +98
if len(sys.argv) < 2:
print("Usage: command_runner.py <command> [args...]", file=sys.stderr)
sys.exit(1)

command = sys.argv[1:]
Copy link
Contributor

Choose a reason for hiding this comment

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

better to use the argparse module here, it's more standard for python. it can also let you take in an arbitrary number of inputs.

Copy link
Contributor

@itsarune itsarune left a comment

Choose a reason for hiding this comment

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

looks good, I think you just need to update the workflow YAMLs and then it should work

)

# Proto log folder name
parser.add_argument(
Copy link
Contributor

Choose a reason for hiding this comment

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

nice work!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants