Skip to content
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

Merged
merged 10 commits into from
Mar 3, 2025

Conversation

y3rsh
Copy link
Member

@y3rsh y3rsh commented Feb 27, 2025

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.

  • protocols that have _S_ in their name or are from the protocol library should not have errors in their analysis
  • protocols that have _X_ in their name are expected to have errors.

Notes

  • Made a matrix tool to analyze protocols against release versions of the Robot stack.
    image
  • Fix all 17 😢 mismatches creating bugs and/or updating the snapshot/protocols.

Bugs

image

@y3rsh y3rsh added the gen-analyses-snapshot-pr Generate a healing PR if the analyses snapshot test fails label Feb 27, 2025
@y3rsh y3rsh self-assigned this Feb 27, 2025
@y3rsh y3rsh changed the title fix(ab): add test to verify analysis outcome matches analysis output fix(ab): verify protocol test intention matches analysis output Feb 27, 2025
Copy link
Contributor

github-actions bot commented Mar 2, 2025

A PR has been opened to address analyses snapshot changes. Please review the changes here: #17625

Copy link
Contributor

github-actions bot commented Mar 3, 2025

A PR has been opened to address analyses snapshot changes. Please review the changes here: #17626

Copy link

codecov bot commented Mar 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 25.68%. Comparing base (7d35e6f) to head (2909cdc).

Additional details and impacted files

Impacted file tree graph

@@           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           
Flag Coverage Δ
protocol-designer 18.94% <ø> (ø)

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

@y3rsh y3rsh marked this pull request as ready for review March 3, 2025 13:39
@y3rsh y3rsh requested review from a team as code owners March 3, 2025 13:39
Comment on lines +46 to +49
# which is best to check???
# they both give the same result
# errors_present = data["errors"] != []
errors_present = data["result"] != "ok"
Copy link
Member Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

They should be equivalent.

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a 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?

…TTES_DescriptionTooLongRTP.py

Co-authored-by: Max Marrone <max@opentrons.com>
@y3rsh y3rsh merged commit 3cd9bb4 into edge Mar 3, 2025
7 checks passed
@y3rsh y3rsh deleted the analyses-snapshot-audit branch March 3, 2025 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gen-analyses-snapshot-pr Generate a healing PR if the analyses snapshot test fails
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants