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

test(analyses-snapshots): update analyses snapshot local instructions #16918

Merged
merged 6 commits into from
Nov 21, 2024

Conversation

y3rsh
Copy link
Member

@y3rsh y3rsh commented Nov 20, 2024

Overview

Fixed the Makefile targets to allow runs against the local code. Updated the README with accurate instructions.

Risk

None, test code only.

@y3rsh y3rsh self-assigned this Nov 20, 2024
@y3rsh y3rsh requested a review from a team as a code owner November 20, 2024 20:37
Comment on lines 80 to 81
`make build-base-image` - build the base image for the analyses battery
`make build-local` - on top of the base, copy in the local code and build
Copy link
Member Author

@y3rsh y3rsh Nov 20, 2024

Choose a reason for hiding this comment

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

Should I have the snapshot-test-local and snapshot-test-update-local targets run the build targets so there are less steps?

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't been around long enough to have an opinion.

But generally speaking, the spirit of make is that make foo should also build all the prerequisites for foo if they don't exist (or rebuild them if they are out-of-date) unless it's way too complicated or it has unexpected side effects.

@@ -4,7 +4,7 @@

1. Follow the instructions in [DEV_SETUP.md](../DEV_SETUP.md)
1. `cd analyses-snapshot-testing`
1. use pyenv to install python 3.12 and set it as the local python version for this directory
1. use pyenv to install python 3.13 and set it as the local python version for this directory
Copy link
Contributor

Choose a reason for hiding this comment

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

What do we use that requires Python 3.13 here, and why do they other dev environment setup instructions recommend 3.10 instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are locked to 3.10 for all python on robots because of reasons I don't know specifically ,but that have to do with the burden of upgrading python version in buildroot and oe-core.
The image running the code under test is 3.10, just like it is in the app and on the robots.
The use of 3.13 is only for the testing tools in this folder. It is faster and is a place we can explore latest and greatest features 😄

snapshot-test-local: ANALYSIS_REF=$(LOCAL_IMAGE_NAME)
snapshot-test-local:
@echo "This target is overriding the ANALYSIS_REF to the local image name: $(LOCAL_IMAGE_NAME)"
@echo "ANALYSIS_REF is $(ANALYSIS_REF). This is the image used in the test."
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't quite understand ANALYSIS_REF after reading the README. Is it just edge at HEAD?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is the variable in the Makefile that is used to tell the build targets which code ref of the opentrons code to build. It is also used as the image name on the built image. The test then uses this variable for which image to start that will produce the analyses.

Comment on lines 80 to 81
`make build-base-image` - build the base image for the analyses battery
`make build-local` - on top of the base, copy in the local code and build
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't been around long enough to have an opinion.

But generally speaking, the spirit of make is that make foo should also build all the prerequisites for foo if they don't exist (or rebuild them if they are out-of-date) unless it's way too complicated or it has unexpected side effects.

@y3rsh y3rsh changed the title Update analyses snapshot local instructions test(analyses-snapshots): update analyses snapshot local instructions Nov 21, 2024
@y3rsh y3rsh merged commit 84a57ce into edge Nov 21, 2024
8 checks passed
@y3rsh y3rsh deleted the update-analyses-snapshot-local-instructions branch November 21, 2024 16:30
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.

2 participants