-
Notifications
You must be signed in to change notification settings - Fork 126
Installation tests v3 #2330
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
base: master
Are you sure you want to change the base?
Installation tests v3 #2330
Conversation
cache-environment: true | ||
post-cleanup: 'all' | ||
|
||
- name: Add arcticdb from conda-forge |
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'll want to run these using the Linux Conda build too, not just Mac
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.
Yes I havethought about it too and would propose to add as one of the combination (currently we have 3 linuxes + pypi, we can make one of linuxes not pypi bu conda)
def lmdb_storage(tmp_path) -> Generator[LmdbStorageFixture, None, None]: | ||
with LmdbStorageFixture(tmp_path) as f: | ||
yield f | ||
def lmdb_storage(request, tmp_path) -> Generator[LmdbStorageFixture, None, None]: |
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 think it will be clearer to have a dedicated test suite for these installability tests because they will have to keep working with possibly very old ArcticDB versions, so will in a sense be "frozen" - whereas the core tests will change more over time.
I also don't think we should add this complexity to the core lmdb_storage
fixture - a dedicated single purpose fixture would be clearer to me.
I understand why you've done it like this, it is quite appealing to just reuse existing tests, and the way you've done it is really neat, I just think it is best to be painfully obvious here (especially for the comprehensibility for new joiners etc).
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 have testsed so far with 4.4.7 version (lowest) and there were no problems. But I do understand what you mean. An approach with what you suggest was the original one - #2316
The idea there was to reuse as little as possible, and therefore only shared_tests.py was the thing that both current tests and new tests shared, and the idea was once a test started to diverge to be no more shared but have two versions - one for the original and one for the installation tests (it was ok since the tests files/suites were different )
Regarding the open questions in the description,
Finally, it will be important to have some brief docs explaining this setup (and why we have it) for the benefit of future developers. |
I think an important part of this will be having a way to constrain the dependencies used by earlier ArcticDB versions - we don't want tests to fail on ArcticDB vPREHISTORIC when numpy 3 is released, for example |
Do we think it's valuable to do this on PyPi (where we bundle all our C++ deps) or perhaps only doing this on Conda would suffice? |
|
name: Run Installation Tests v3 | ||
|
||
on: | ||
push: |
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 looks like we are planning to run those tests on branch push. I think it's much better to run them periodically. We rarely need to update older branches, but we still need to keep the older versions workable.
steps: | ||
|
||
- name: Checkout code | ||
uses: actions/checkout@v3 |
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.
Why do we need to check out? Is it for checking out tests? If we rely on latest tests with older versions of arcticdb, it might not have the up to date functionality.
Reference Issues/PRs
What does this implement or fix?
Those are installation tests targeted to be executed against different versions of arcticdb from conda and pypi without building our project.
GitHubWorkflow (not yet finished)
allows matrix execution of selected combinations of OS-es and Pytho versions. For each it install artcicdb from conda(mac-os) and pypi(other os-es), along with our test dependencies. We build also protobufs and execute our tests
Not finished: ability to select arcticdb version, ability to run on demand with selected options from user
Link to successfull run with LMDB tests only:
latest 5.3.3: https://github.com/man-group/ArcticDB/actions/runs/14494364845
5.1.2: https://github.com/man-group/ArcticDB/actions/runs/14509109094/job/40703855715
4.4.7: https://github.com/man-group/ArcticDB/actions/runs/14509267618
Added:
Modified:
Opened Questions:
Any other comments?
Checklist
Checklist for code changes...