Skip to content

test(robot-server): Test compatibility with databases created in v6.0 #11448

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

Merged
merged 14 commits into from
Sep 20, 2022

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Sep 8, 2022

Overview

This adds a basic test for persistence backwards compatibility, which includes backwards compatibility in our SQL database. The test makes sure that the current robot-server code can read some resources that were created by a v6.0.1 robot-server.

Closes RSS-102.

This is a humongous diff—sorry! Most of it is opaque fixture data. The actual code changes are fairly small.

Changelog

  • Copy a pre-populated persistence directory into the repository, intended to be used as an opaque test fixture. I created it by manually issuing HTTP requests to a v6.0.1 dev server and saving its persistence directory. See the added readme for details.

  • Add a test that runs a dev server underpinned by that persistence directory, issues a bunch of HTTP GET requests to it, and makes sure none of the requests come back with an obvious error.

    For simplicity, we only check the responses in a very shallow way. Basically, we just check that the HTTP status is successful, and that the response data isn't empty. I believe this is sufficient to catch the bugs that we've been prone to so far.

Review requests

  • Will these tests give us sufficient confidence to rework our persistence code according to the plan in RSS-98?
  • Try making a change in robot-server, api, or shared-data that would break backwards compatibility. For example, try making an Optional field in a persisted Pydantic model non-Optional. Then, run make -C robot-server test—does it catch the problem?
  • Do you have any ideas for future improvements? If so, please record them in RSS-100.

Risk assessment

No risk to production code. Changes are only to tests.

"mount": "right"
"mount": "left"
Copy link
Contributor Author

@SyntaxColoring SyntaxColoring Sep 8, 2022

Choose a reason for hiding this comment

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

This looked like a mistake in the fixture. Both pipettes were being loaded on the right mount, which caused simulation and run problems. Elsewhere in this file, within designerApplication, this pipette is said to be on the left mount, so I changed this to match.

@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #11448 (569754e) into edge (715b2a0) will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #11448      +/-   ##
==========================================
- Coverage   74.31%   74.22%   -0.09%     
==========================================
  Files        2072     2083      +11     
  Lines       57377    57468      +91     
  Branches     5539     5550      +11     
==========================================
+ Hits        42638    42655      +17     
- Misses      13482    13555      +73     
- Partials     1257     1258       +1     
Flag Coverage Δ
app 74.20% <ø> (-0.44%) ⬇️
protocol-designer 45.81% <ø> (ø)
shared-data 86.44% <ø> (ø)
step-generation 88.39% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
app/src/organisms/ChangePipette/Instructions.tsx 76.92% <0.00%> (-0.86%) ⬇️
app/src/App/index.tsx 100.00% <0.00%> (ø)
app/src/molecules/Modal/index.tsx 100.00% <0.00%> (ø)
app/src/atoms/StatusLabel/index.tsx 100.00% <0.00%> (ø)
.../python/opentrons_shared_data/pipette/dev_types.py 100.00% <0.00%> (ø)
app/src/atoms/buttons/index.tsx
app/src/atoms/Toast/Toast.stories.tsx 0.00% <0.00%> (ø)
app/src/atoms/buttons/SubmitPrimaryButton.tsx 100.00% <0.00%> (ø)
app/src/organisms/CalibrationStatusCard/index.tsx 66.66% <0.00%> (ø)
...p/src/pages/Devices/CalibrationDashboard/index.tsx 80.00% <0.00%> (ø)
... and 10 more

@SyntaxColoring SyntaxColoring marked this pull request as ready for review September 9, 2022 21:05
@SyntaxColoring SyntaxColoring requested review from a team as code owners September 9, 2022 21:05
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

A few little thoughts but this seems like a pretty vast improvement in our regression detection abilities for these APIs

Copy link
Contributor

@TamarZanzouri TamarZanzouri left a comment

Choose a reason for hiding this comment

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

Awesome work Max! Thank you!

@SyntaxColoring SyntaxColoring merged commit 7273aa6 into edge Sep 20, 2022
@SyntaxColoring SyntaxColoring deleted the test_persistence_compatibility branch September 20, 2022 14:16
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.

3 participants