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

Add integration test with Manifolds #58

Open
sethaxen opened this issue Jan 16, 2021 · 8 comments
Open

Add integration test with Manifolds #58

sethaxen opened this issue Jan 16, 2021 · 8 comments

Comments

@sethaxen
Copy link
Member

We should have an integration test with Manifolds, that is, an action that runs Manifolds's test suite using the latest changes to ManifoldsBase. This will ensure that potentially breaking changes are identified before being merged. This workflow is really nice: JuliaDiff/ChainRulesTestUtils.jl#105

@kellertuer
Copy link
Member

This means on a PR we run a complete test suite on Manifolds? That would increase test time like a lot. What do you think about making it optional (similar to documentation building over on Manifolds.jl)? Or does that act against what the integration should do?

@sethaxen
Copy link
Member Author

sethaxen commented Mar 8, 2021

No, it would be a separate workflow. See for example the one linked above. So it would not alter the time it takes to run the current test suite, and we would only need to pay attention to the integration test result before merging a PR. It provides extra security against making breaking changes, and for that reason, I don't think it should be optional.

@kellertuer
Copy link
Member

Ok, but what I mean is, if you open a PR, then all tests run, including the integration test. All tests here in total take about 4 minutes or something, but the integration test would run the whole Manifolds.jl tests, right? So that test would take 35 minutes.
That's why I asked. Since I did the adaption for Manopt integration with Manifolds, for sure I can easily do this one, too. I just wanted to clarify this point beforehand.

@sethaxen
Copy link
Member Author

sethaxen commented Mar 8, 2021

All tests here in total take about 4 minutes or something, but the integration test would run the whole Manifolds.jl tests, right? So that test would take 35 minutes.

That's right, yes.

@kellertuer
Copy link
Member

Could. we maybe introduce a short integration test over on Manifolds that we could test against? I do not want to test all manifolds with all variants and different static arrays and such.

@kellertuer
Copy link
Member

A short update: I started reworking our test suite at JuliaManifolds/Manifolds.jl#548 so that we avoid a few double checks we are currently doing, but also such that the tests get more modular. Then we can maybe also do an integration test setup (maybe testing some major manifolds Rn, Sphere, Hyperbolic, Stiefel, SPD?) which to together do not run too long and still provide a good integration test.

@mateuszbaran
Copy link
Member

I personally wouldn't mind a long-running integration test in ManifoldsBase.jl but a smaller version would be better, yes.

@kellertuer
Copy link
Member

I think it depends, when the integration takes the current hour the Manifolds.jl tests run compared to the few minutes ManifoldsBase.jl runs its a little annoying.

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 a pull request may close this issue.

3 participants