pepkit / looper

A job submitter for Portable Encapsulated Projects
http://looper.databio.org
BSD 2-Clause "Simplified" License
20 stars 7 forks source link

Simplify tests #353

Closed nsheff closed 7 months ago

nsheff commented 1 year ago

Right now running pytest on the dev branch takes 90 sec on my local computer

I'm open to other opinions, but in my opinion, if the tests take more than 15 seconds or so, then I don't run them as often and they lose some utility.

Given that looper doesn't do that much complicated stuff I'm wondering if we can pare down the tests to complete more quickly.

donaldcampbelljr commented 1 year ago

The slowest tests exist under smoketests. Typically, a single test runs a subprocess of looper via subprocess.Popen, i.e. subprocess.Popen(looper run config), captures any stderr/stdout and asserts there is no error. Unfortunately, many of these simple tests take more than 1 second to complete:

================================================== slowest durations ===================================================
2.20s call     tests/smoketests/test_run.py::TestLooperRunPreSubmissionHooks::test_looper_other_plugins[looper.write_sample_yaml_cwl-cwl.yaml]
1.99s call     tests/smoketests/test_run.py::TestLooperConfig::test_init_config_file[run]
1.81s call     tests/smoketests/test_run.py::TestLooperBothRuns::test_cmd_extra_cli[arg0-run]
1.63s call     tests/smoketests/test_run.py::TestLooperRunPreSubmissionHooks::test_looper_other_plugins[looper.write_submission_yaml-submission.yaml]
1.57s call     tests/smoketests/test_run.py::TestLooperRunBehavior::test_looper_multi_pipeline
1.56s call     tests/smoketests/test_run.py::TestLooperRunPreSubmissionHooks::test_looper_command_templates_hooks[touch {looper.output_dir}/submission/{sample.sample_name}_test.txt; {%raw%}echo {}{%endraw%}]
1.53s call     tests/smoketests/test_run.py::TestLooperCompute::test_cli_yaml_settings_passes_settings[run]
1.53s call     tests/smoketests/test_run.py::TestLooperRunBehavior::test_cmd_extra_override_sample[ --sjhsjd 212]
1.53s call     tests/smoketests/test_run.py::TestLooperRunBehavior::test_looper_run_basic
1.52s call     tests/smoketests/test_run.py::TestLooperBothRuns::test_cmd_extra_cli[arg1-run]
1.51s call     tests/smoketests/test_run.py::TestLooperCompute::test_nonexistent_yaml_settings_disregarded[run]
1.51s call     tests/smoketests/test_run.py::TestLooperRunBehavior::test_cmd_extra_sample[ --sjhsjd 212]
1.51s call     tests/smoketests/test_run.py::TestLooperRunPreSubmissionHooks::test_looper_other_plugins[looper.write_sample_yaml_prj-prj.yaml]
1.51s call     tests/smoketests/test_run.py::TestLooperRunSubmissionScript::test_looper_lumping
1.49s call     tests/smoketests/test_run.py::TestLooperRunBehavior::test_cmd_extra_sample[ --string]
1.48s call     tests/smoketests/test_run.py::TestLooperRunBehavior::test_cmd_extra_sample[string]
1.47s call     tests/smoketests/test_run.py::TestLooperRunBehavior::test_cmd_extra_override_sample[7867#$@#$cc@@]
1.46s call     tests/smoketests/test_run.py::TestLooperRunBehavior::test_cmd_extra_override_sample[ --string]
1.46s call     tests/smoketests/test_run.py::TestLooperRunBehavior::test_looper_sample_attr_missing
  1. We could remove some of these tests (e.g. do we need both test_cmd_extra_sample conditions?).
  2. We could also consider using pytest-xdist which allows pytest to distribute and run the tests in parallel over multiple CPUs.

On my machine this drops the execution time from 114 secs to 24 secs.

donaldcampbelljr commented 1 year ago

Discussed:

donaldcampbelljr commented 1 year ago

Some tests were changed to call main directly instead of using subprocess.Popen. This change allows for decreasing test time from 78 secs to 43 secs. Some tests still need to use the subprocess.Popen because they must assert that certain values exist in stdout. The current implementation appears to be best practice and other methods to retrieve and decode stdout proved difficult to implement.

Therefore, I am marking this as likely solved with the PR #386.

donaldcampbelljr commented 11 months ago

Re-opening for now. Currently doing another pass to remove the remaining subprocess calls and instead use a returned dict that contains important messages from the looper run. See commit https://github.com/pepkit/looper/commit/a29b7b8a0301492506b6973f88003cd417de6998 as an example.

donaldcampbelljr commented 7 months ago

Closed via https://github.com/pepkit/looper/pull/400