https://nf-co.re logo
Join Slack
Powered by
Test config parameters reorganisation (use same or...
# request-review
l
Test config parameters reorganisation (use same order for all nf-test) for phaseimpute. No change in snapshot. https://github.com/nf-core/phaseimpute/pull/278
✅ 1
Thanks for the quick review @Maxime Garcia!!
m
It was an easy one, as you explained most of it was reorganisation
l
I now want to update all the test to use the genetic map. For each tool I can either provide the chunks.csv and the map.csv or not. This means 4 different test (00, 01, 10, 11) but this might be a bit too much. What do you think ?
m
iep
how many tools do you have?
And is it already tested somewhere else that they work with/without these CSV files?
if so, no need to overdo it
l
6 tools glimpse1, glimpse2, quilt, stitch, minimac4, beagle5 I also have the panelprep test that is impacted by the presence/absence of these files. The test_all use 
glimpse1,glimpse2,stitch,quilt
but is meant to see if all the steps can be run at once with different tools. I know that Anabella like to keep the old test, but we start to have quite a lot of them, and I'm not sure all of them are necessary. Maybe I should simplify into, all and no optional file given (00 vs 11) ?
If i add the 4 possibilities for each tool and panelprep test, we would have 34 tests at each PR, if I use only the 2 extreme case we would have 20 tests.
m
For me, CI tests should be quick enough
if you really want to test if things work with or without files, that's just channel logic, so maybe just stub tests would be enough for that
l
I agree that CI should be fast. However I guess the stub test wouldn't be enough as both files have a huge impact on the computation, but this could be worth the try. I've open a PR and asked Anabella her point of view. On my side I would simply go for 00 vs 11. https://github.com/nf-core/phaseimpute/pull/279
3 Views
Open in Slack
PreviousNext
Powered by