-
Notifications
You must be signed in to change notification settings - Fork 179
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.
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
test(robot-server): Deduplicate and tidy up integration test fixtures #12795
Conversation
I suspect this was left over from a bad merge, or something.
Resolve conflicts in test_get_loaded_labware_definitions.tavern.yaml.
Codecov Report
@@ Coverage Diff @@
## edge #12795 +/- ##
==========================================
- Coverage 73.50% 73.49% -0.01%
==========================================
Files 2261 2271 +10
Lines 62047 62159 +112
Branches 6717 6736 +19
==========================================
+ Hits 45605 45684 +79
- Misses 14842 14867 +25
- Partials 1600 1608 +8
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the right call to me. Explicit is good but in addition to the copy pasting issue, forgetting the clean fixture would lead to nonlocal failures which are awful to debug. I think the downsides outweigh the upsides.
Overview
Some Friday zen refactoring because I needed something easy and gratifying. This tidies up
robot-server
's integration test infrastructure.Notable changes
Less URL construction
Integration tests no longer have to repeat the
{host:s}:{port:d}
thing for every request. They can now use{ot2_server_base_url}
or{ot3_server_base_url}
.Less error-prone test isolation
Before this PR, each integration test had to remember to use
clean_server_state
to reset the shared dev server. Otherwise, the runs and protocols that it added would leak into subsequent tests.Writing the tests this way was a deliberate choice in #12193 for explicitness, but I think it was turning out to be too easy to forget, especially with how much copy-pasting we do in Tavern files.
With this PR, that cleanup is now built in to the dev server fixture itself. This gives us test isolation by default, which is what we normally expect in pytest.
Less duplication
This resolves a bit of duplication that had grown over time. A lot of this was my fault, like #12595 (comment).
Review requests
You definitely do not have to review all 60+ files. The vast majority of the changes are simple renames. Here are the most meaningful files:
Test plan
Make sure CI keeps passing.
Risk assessment
Low. Changes are only to tests.