Test restructure and parallelisation #835
Conversation
| @test isequal(ModelingToolkit.get_iv(rn), s) | ||
| @test issetequal(Catalyst.get_sivs(rn), [x]) | ||
| end | ||
|
|
There was a problem hiding this comment.
These are DSL options, so I moved them to the file for DSL option tests. We probably should add some additional @variables tests at some point, and maybe for @ivs tests as well.
| weak_rev = false | ||
| testreversibility(rn, reactioncomplexes(rn)[2], rev, weak_rev) | ||
| end | ||
|
|
There was a problem hiding this comment.
Moved to network analysis tests. There were quite a few things in the API test file that I think maybe should be moved, but was honestly unsure. Only moved this and the conservation law stuff because those where the only ones I felt very certain about.
| @test isapprox(g1, g2[istsidxs, :]) | ||
| @test isapprox(g2[istsidxs, :], g3) | ||
| end | ||
|
|
There was a problem hiding this comment.
Moves to conservation laws test file.
| @@ -1,74 +0,0 @@ | |||
| #! format: off | |||
There was a problem hiding this comment.
This is tested, and in waaay more depth, in the new upstream test files.
| @@ -1,34 +0,0 @@ | |||
| #! format: off | |||
There was a problem hiding this comment.
The empty test network test was moved to the reaction system structure test. This file is pretty much deprecated when we removed mutating functions, and hence removed.
It feels silly and bad that we end up with a folder with only a single file (the composing model test). However, I'd like to test more for hierarchical/composed systems, so we will likely add more test files here? In which case it would make sense to keep this one.
| str = String(take!(io)) | ||
| @test count(isequal('\n'), str) < 30 | ||
| end | ||
|
|
There was a problem hiding this comment.
I have grouped show tests together.
| rx3 = Reaction(2 * k, [B], [D], [2.5], [2]) | ||
| @named mixedsys = ReactionSystem([rx1, rx2, rx3], t, [A, B, C, D], [k, b]) | ||
| mixedsys = complete(mixedsys) | ||
| osys = convert(ODESystem, mixedsys; combinatoric_ratelaws = false) |
There was a problem hiding this comment.
I have grouped boundary condition tests together (still in the same file).
| str = String(take!(io)) | ||
| @test str == "a, A + 2*(C(t))[1] --> 2*(C(t))[2] + 3*B" | ||
| end | ||
|
|
There was a problem hiding this comment.
I have grouped show tests together.
| @test_throws ErrorException ReactionSystem([rx], t; name = :rs) | ||
| @named rs = ReactionSystem([rx], t; balanced_bc_check = false) | ||
| end | ||
|
|
There was a problem hiding this comment.
I have grouped boundary condition tests together (still in the same file).
There was a problem hiding this comment.
My only concern is the sheer number of groups and whether that has implications on test time potentially taking longer, especially as some groups have very small numbers of tests within them...
Do you have any quantitative information on how this impacts total time for all tests to run?
.github/workflows/CI.yml
Outdated
| - Core | ||
| - ReactionSystem | ||
| - DSL | ||
| - CompositionalModelling | ||
| - Miscellaneous | ||
| - NetworkAnalysis | ||
| - Simulation | ||
| - Upstream | ||
| - Spatial | ||
| - Visualisation | ||
| - Extensions |
There was a problem hiding this comment.
I don't know groups too well, but could this actually result in things getting queued when SciML is pushing the number of concurrent CI runs and actually making everything take longer? This seems like a lot of groups.
@ChrisRackauckas is this a reasonable or too many? Some of these groups only run like one or two very short tests. Does each group require a separate setup/initialization process?
There was a problem hiding this comment.
Yes, those are good questions. Once we have figured this out we can separate the tests into an appropriate number of groupings.
|
I kind of agree. When I did it I didn't realise there would be a separate run for each group. Maybe we create separate groups (independent from the previous grouping), and just split it into 3-4 sets depending on runtimes? |
|
I think that makes more sense. Something like all the symbolic DSL/ReactionSystem stuff, all the solving stuff, and the extensions seems like a natural split. We can further split solving stuff if needed to model types or such. |
|
Agreed. For now, maybe just call stuff "TestGroup1", "TestGroup2", etc.? That way we don't have to be super committed to what goes into which group. |
|
Sure. Changing around the group names later doesn’t seem like a big deal anyways — it isn’t user facing in any way. |
| @test length(ps) == 0 | ||
| empty_network = @reaction_network | ||
| @test length(equations(empty_network)) == 0 | ||
| @test nameof(independent_variable(empty_network)) == :t |
There was a problem hiding this comment.
This is a deprecated function. More generally why have you dropped using the getters here? We need to test them too.
There was a problem hiding this comment.
There was a problem hiding this comment.
Didin't realise that the test was specific to using the get_eqs getter instead of the equations getters. Figured it was just about checking the content of the empty network. If this is and important part of the test, shouldn't we just use the full set?
Good point about independent_variable, I will revert that and the others.
Do not change any actual code. However: