Runtime selection of field vs simulated gameplay tests#3744
Conversation
nycrat
force-pushed
the
avah/combine_simulated_and_field_tests
branch
5 times, most recently
from
f7b544d to
db5f8e2
Compare
nycrat
force-pushed
the
avah/combine_simulated_and_field_tests
branch
from
db5f8e2 to
fd08dcd
Compare
nycrat
force-pushed
the
avah/combine_simulated_and_field_tests
branch
from
d5f2675 to
a49d254
Compare
| while True: | ||
| try: | ||
| world = self.world_buffer.get( | ||
| block=True, timeout=WORLD_BUFFER_TIMEOUT |
There was a problem hiding this comment.
Hi! From my understanding whne the test is waiting for a world message and none arrive in 5 seconds it logs ending test early but it doesn't actually end early (just logs message and tries waiting again... could just wait forever if no world shows up?) I don't think the timeout gets checked when it's stuck in this loop. I think this is cause the code was copied from simulated test runner where retrying made sense... on each retry we re send the missing data so it can probably eventually fix the problem but in field runner that resend logic is missing.
There was a problem hiding this comment.
Yeah I think your logic makes sense. This exact code was from like 3 years ago when field test runner was initially created so idk how this bug was not spotted yet. I'll write a fix so that the test actually stops
| import pytest | ||
|
|
||
|
|
||
| def get_runtime_dir(): |
There was a problem hiding this comment.
doesn't this break the CI uploads? The old code put test files in /tmp/tbots locally but this generates a random folder. But CI looks for log files in /tmp/tbots/blue/... and /tmp/tbots/yellow/... which show up in randomly named folders... I don't think CI can find them?
Could we bring back TEST_TMPDIR symlink behavior, default to /tmp/tbots locally like before. Then add something random to the path if you actually need to run tests at the same time?
There was a problem hiding this comment.
Oh nice catch I actually didn't know about this. I think this should be separated into another ticket cause this behaviour of get_runtime_dir was already implemented before this PR. I'm thinking of putting the randomly generated folders in /tmp/tbots/ cause its still necessary in ci_mode to have different folders
|
@annieisawesome2 I addressed comments, ready for re-review if you have time |
nycrat
force-pushed
the
avah/combine_simulated_and_field_tests
branch
from
fa2aba5 to
e42ab95
Compare
Sounds good I will finish up tonight |
nycrat
force-pushed
the
avah/combine_simulated_and_field_tests
branch
from
8fd923b to
42e8972
Compare
| time_elapsed_s += time.time() - processing_start_time | ||
|
|
||
| # Check that all eventually validations are eventually valid | ||
| validation.check_validation(eventually_validation_proto_set) |
There was a problem hiding this comment.
All the new changes look great! One small thing with the world timeout fix. If no world is received, here it references eventually_validation_proto_set before it is assigned i think. we can probs just fix with pytest.fail.. in the except queue.Empty block?
There was a problem hiding this comment.
You're right I didn't notice that. I rewrote it to raise an exception since that is the same mechanism the test validations use to signal a test failure
3 participants
Description
This is a stacked PR: #3744 #3806 #3807
This PR combines simulated and field gameplay test runners under the common TbotsTestRunner interface. This allows all tests to be run as either field tests or simulated tests with just a different flag passed into the
bazel testcommand.This PR contains many refactoring changes to our existing python testing framework. These changes were important to improving code quality while implementing the changes, and they will make it easier to implement #3369 in the future.
./tbots.py test move_tactic_test./tbots.py test move_tactic_field_test./tbots.py test move_tactic_test --run_field_test./tbots.py test move_tactic_field_test --run_field_testTesting Done
Resolved Issues
resolves #2908
resolves #2856 cause this PR just gets rid of simulated aggregate tests
resolves #3784
Length Justification and Key Files to Review
A lot of just renaming and combining the simulated_tests and field_tests under the gameplay_tests package. None of the logic is changed for validations and existing tests.
Important not just refactoring changes to review:
(in src/software/gameplay_tests)
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.hfile) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO(or similar) statements should either be completed or associated with a github issue