-
Notifications
You must be signed in to change notification settings - Fork 14
Add integration tests #149
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
Conversation
giordano
left a comment
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.
Yay! My only concern for you is that this will add a bit of precompilation latency for all the tests, but at least it's a real world large-ish codebase with many interesting corner cases.
| [compat] | ||
| Oceananigans = "=0.103.1" |
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.
Good to strictly fix the version, because I'm slowly but steadily going through all the modules to fix everything 😁
Co-authored-by: Mosè Giordano <765740+giordano@users.noreply.github.com>
|
I've put them in a separate CI workflow, and just pushed a commit to parallelize over packages (though there's only 1 currently) so hopefully it's not too bad. My plan is to not run them when testing locally since failures here are unexpected (our normal tests should catch most things) but they'll run in PRs to catch issues before merge (hopefully). |
|
Ok, it takes 3 minutes in total starting from scratch, it's not too bad after all. CUDA (which depends on DataFrames, which takes a while) is a pkgextension nowadays in Oceananigans. |
I've been making some changes and was worried about introducing issues that cause failures to package CI. So I thought I'd add some tests. Here I've added Oceananigans, cc @giordano.