search

opencobra / memote

memote – the genome-scale metabolic model test suite
https://memote.readthedocs.io/
Apache License 2.0
125 stars 26 forks source link

Cli tests #616

Closed ChristianLieven closed 5 years ago

ChristianLieven commented 5 years ago
codecov[bot] commented 5 years ago

Codecov Report

Merging #616 into develop will increase coverage by 4.3%. The diff coverage is 25.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop     #616     +/-   ##
==========================================
+ Coverage    80.03%   84.34%   +4.3%     
==========================================
  Files           59       59             
  Lines         3131     3219     +88     
  Branches       428      429      +1     
==========================================
+ Hits          2506     2715    +209     
+ Misses         552      415    -137     
- Partials        73       89     +16
Impacted Files Coverage Δ
memote/suite/cli/reports.py 75.37% <100%> (+28.45%) :arrow_up:
memote/suite/cli/runner.py 50.71% <22.83%> (+8.07%) :arrow_up:
memote/suite/tests/test_thermodynamics.py 88.88% <0%> (-11.12%) :arrow_down:
memote/suite/collect.py 83.33% <0%> (+5.55%) :arrow_up:
memote/suite/api.py 69.64% <0%> (+7.14%) :arrow_up:
memote/suite/results/result_manager.py 89.28% <0%> (+14.28%) :arrow_up:
memote/suite/results/repo_result_manager.py 94.87% <0%> (+17.94%) :arrow_up:
memote/jinja2_extension.py 73.68% <0%> (+21.05%) :arrow_up:
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f50986c...8b0c92a. Read the comment docs.

codecov[bot] commented 5 years ago

Codecov Report

Merging #616 into develop will increase coverage by 4.59%. The diff coverage is 29.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop    #616      +/-   ##
==========================================
+ Coverage    80.81%   85.4%   +4.59%     
==========================================
  Files           59      59              
  Lines         3132    3234     +102     
  Branches       429     434       +5     
==========================================
+ Hits          2531    2762     +231     
+ Misses         532     384     -148     
- Partials        69      88      +19
Impacted Files Coverage Δ
memote/suite/cli/reports.py 75.37% <100%> (+28.45%) :arrow_up:
memote/suite/cli/runner.py 53.67% <25.87%> (+10.92%) :arrow_up:
memote/suite/reporting/history.py 98.03% <0%> (+3.92%) :arrow_up:
memote/experimental/config.py 81.08% <0%> (+5.4%) :arrow_up:
memote/suite/api.py 69.64% <0%> (+7.14%) :arrow_up:
memote/suite/results/result_manager.py 89.28% <0%> (+14.28%) :arrow_up:
memote/suite/results/repo_result_manager.py 94.87% <0%> (+17.94%) :arrow_up:
memote/jinja2_extension.py 73.68% <0%> (+21.05%) :arrow_up:
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8c4eabc...2bbcad3. Read the comment docs.

ChristianLieven commented 5 years ago

Really nice to see this increase in test coverage! I've added some minor comments inline just for consideration, but I also have two slightly more important concerns:

  • The mock_repo fixture clones from your private account on GitHub. That adds a couple of seconds to each test requiring this repo, and assumes your mock repo will always exist. Would it be possible to clone it from test data, like with the models? (I realize this is perhaps outside of the scope of this PR, but I'd suggest maybe creating a new issue for it.)
  • The runtime of the test suite has gone up quite a bit. test_new and test_history alone adds 23 seconds. I would recommend to try to optimize the tests. If that's not possible, then consider flagging the very slow tests so that they're maybe not included in the local test suite by default, but are still run on CI. See the test duration summary below (note that the setup timings are mostly due to the mock_repo fixture):
18.65s call     tests/test_for_suite/test_for_cli/test_for_runner.py::test_new
4.37s call     tests/test_for_suite/test_for_cli/test_for_runner.py::test_history
4.06s call     tests/test_for_suite/test_for_cli/test_for_runner.py::test_run_simple[ecoli-core]
3.79s call     tests/test_for_suite/test_for_cli/test_for_reports.py::test_diff[ecoli-core]
3.74s call     tests/test_for_suite/test_for_cli/test_for_runner.py::test_run_no_repo_ignore_git_false[ecoli-core]
3.68s call     tests/test_for_suite/test_for_cli/test_for_runner.py::test_run_output[ecoli-core]
2.94s call     tests/test_for_suite/test_for_cli/test_for_reports.py::test_snapshot[ecoli-core]
2.83s call     tests/test_for_suite/test_for_api.py::test_test_model_code[glpk-complete_failure-1]
2.83s call     tests/test_for_suite/test_for_cli/test_for_runner.py::test_run_skip_unchanged_false
2.79s setup    tests/test_for_suite/test_for_cli/test_for_runner.py::test_run_dirty_repo_ignore_git_false
2.66s call     tests/test_for_suite/test_for_api.py::test_test_model_result[glpk-complete_failure]
2.61s setup    tests/test_for_suite/test_for_cli/test_for_reports.py::test_history
2.61s call     tests/test_for_suite/test_for_api.py::test_snapshot_report_file[glpk-complete_failure]
1.85s setup    tests/test_for_suite/test_for_cli/test_for_runner.py::test_run_no_location
1.54s setup    tests/test_for_suite/test_for_cli/test_for_runner.py::test_history
1.44s setup    tests/test_for_suite/test_for_cli/test_for_runner.py::test_run_skip_unchanged_false
1.39s setup    tests/test_for_suite/test_for_cli/test_for_runner.py::test_run_skip_unchanged_true

To combat both, would I have to use git submodules then? And what about the drawbacks/ implications that has?

https://stackoverflow.com/questions/4659549/maintain-git-repo-inside-another-git-repo

kvikshaug commented 5 years ago

To combat both, would I have to use git submodules then? And what about the drawbacks/ implications that has?

https://stackoverflow.com/questions/4659549/maintain-git-repo-inside-another-git-repo

No, I wouldn't add it as a submodule. I think it's simplest to store the repo as a zip/tarball and in the fixture just uncompress it to a temporary folder.

ChristianLieven commented 5 years ago

To combat both, would I have to use git submodules then? And what about the drawbacks/ implications that has? https://stackoverflow.com/questions/4659549/maintain-git-repo-inside-another-git-repo

No, I wouldn't add it as a submodule. I think it's simplest to store the repo as a zip/tarball and in the fixture just uncompress it to a temporary folder.

OK, that seems reasonable.