-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: isolate test suite #99
Conversation
This commit updates the test-B01_SL_load_single_file.yml workflow file. It changes the name of the workflow to "Test steps" and updates the runs-on value to "ubuntu-latest". Additionally, it adds a step to enable Git LFS and installs pytest and its dependencies. Finally, it updates the run command for the first step and adds a new step to run tests using pytest.
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.
How you feel about renaming the CI workflow file? test.yml
is too general.
I am open. What would be a better name? Naming things is sometimes tricky! @kmilo9999 |
what about |
I like that name! Do we anticipate having other pipelines other than the one for the icesat2 package? @kmilo9999 |
I dont know. We can rename the file again if new pipelines appear and there's a way to distinguish between them. |
Do you believe in YAGNI? @kmilo9999 |
I do believe in it. I just want to have a clear idea that the file runs the pipeline's tests. |
Review and merge this PR after CLI full integration. |
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.
(Just marking this as needing changes – uncommenting the commented-out tests – so it disappears from the list of needed reviews. Thanks!)
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.
Hi Carlos, thanks for this! It looks like a well thought through method of isolating the testing of each of the steps.
I've got some suggestions for simplifications and changes, and some requests for more info!
Use it to define script1
…icesat2-tracks into 96-isolate-test-suite
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.
The code looks good and it seems like many suggestions have already been incorporated. I just found some small text cleanups.
Co-authored-by: Timothy Divoll <[email protected]>
Thanks @tdivoll! |
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.
Just a couple formatting things that seem like they didn't come through in the last commit. I unresolved a couple of the comments to make it easier to find them.
Co-authored-by: Timothy Divoll <[email protected]>
Co-authored-by: Timothy Divoll <[email protected]>
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.
Looks good to me!
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.
looks good to me
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.
Seems to work!
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.
The changes to this file are no longer related to the main change, so we could split these out. But I think they are fine.
Once PRs #90, #92 , #94 (cli's for steps 2-4) are merged the commented code in
test_steps.py
can be uncommented and then update the CI workflow to run all tests via the test suite.#96