-
Notifications
You must be signed in to change notification settings - Fork 178
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
Conversation
analyses-snapshot-testing/README.md
Outdated
`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 |
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.
Should I have the snapshot-test-local
and snapshot-test-update-local
targets run the build targets so there are less steps?
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.
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 |
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 do we use that requires Python 3.13 here, and why do they other dev environment setup instructions recommend 3.10 instead?
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.
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 😄
analyses-snapshot-testing/Makefile
Outdated
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." |
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.
I still don't quite understand ANALYSIS_REF
after reading the README. Is it just edge
at HEAD?
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.
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.
analyses-snapshot-testing/README.md
Outdated
`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 |
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.
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.
Overview
Fixed the
Makefile
targets to allow runs against the local code. Updated the README with accurate instructions.Risk
None, test code only.