-
Notifications
You must be signed in to change notification settings - Fork 180
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
fix(ab): verify protocol test intention matches analysis output #17611
Conversation
A PR has been opened to address analyses snapshot changes. Please review the changes here: #17625 |
A PR has been opened to address analyses snapshot changes. Please review the changes here: #17626 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #17611 +/- ##
=======================================
Coverage 25.68% 25.68%
=======================================
Files 2850 2850
Lines 219210 219210
Branches 17955 17955
=======================================
Hits 56300 56300
Misses 162895 162895
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more. |
# which is best to check??? | ||
# they both give the same result | ||
# errors_present = data["errors"] != [] | ||
errors_present = data["result"] != "ok" |
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.
What is the right test here?
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.
They should be equivalent.
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.
Makes sense, thanks.
Your call, but do you want to mention the X
/S
naming convention in the analysis-snapshot-testing readme? Maybe under the ### Add some protocols to the analyses battery
section?
analyses-snapshot-testing/files/protocols/Flex_X_v2_18_NO_PIPETTES_DescriptionTooLongRTP.py
Outdated
Show resolved
Hide resolved
…TTES_DescriptionTooLongRTP.py Co-authored-by: Max Marrone <max@opentrons.com>
Analyses Snapshot Tests
Add explicit test to validate snapshot error state.
Important
When we review changes to snapshots it is not obvious enough when a snapshot that is supposed to have errors no longer does, and vice versa.
_S_
in their name or are from the protocol library should not have errors in their analysis_X_
in their name are expected to have errors.Notes
Bugs