8185 - Refactor test #8231
8185 - Refactor test #8231
Conversation
Performance Before: 94.81s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_default_value_1_model 20.95s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_default_value_0_ 15.26s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_export_2_model 14.86s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_default_value_2_model 14.55s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_export_1_model 14.28s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_export_0_ Performance after: 1.62s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_ckpt_export_2_model 1.25s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_ckpt_export_default_2_model 0.64s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_ckpt_export_0_ 0.57s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_ckpt_export_1_model 0.57s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_ckpt_export_default_1_model 0.55s call tests/test_bundle_ckpt_export.py::TestCKPTExport::test_ckpt_export_default_0_ 0.01s setup tests/test_bundle_ckpt_export.py::TestCKPTExport::test_ckpt_export_0_
| cmd = ["coverage", "run", "-m", "monai.bundle", "ckpt_export", "network_def", "--filepath", ts_file] | ||
| cmd += ["--meta_file", meta_file, "--config_file", f"['{config_file}','{def_args_file}']", "--ckpt_file"] | ||
| cmd += [ckpt_file, "--key_in_ckpt", key_in_ckpt, "--args_file", def_args_file] | ||
| if use_trace == "True": | ||
| cmd += ["--use_trace", use_trace, "--input_shape", "[1, 1, 96, 96, 96]"] | ||
| command_line_tests(cmd) |
There was a problem hiding this comment.
Part of this test is to check that the command line program also works, this isn't being tested now because you're calling ckpt_export directly. This is fine for checking that the function works, but we should have one test at least (in a separate method?) to check that the command line invocation still functions.
There was a problem hiding this comment.
Humm, I am not sure if I understand correctly, sorry. Would it be necessary to run the full cycle of the test, or would just confirming the command exists and is calling the correct function be enough?
There was a problem hiding this comment.
What the test originally did was run the command coverage run -m monai.bundle ckpt_export ... which I know just calls ckpt_export. I think it would be good to still check that the command still works as a separate test. I would add one test method in that runs this command once for the content of one of the tests just to be sure it doesn't get broken.
There was a problem hiding this comment.
Sure, I understand it was running the command line, but I suppose I don't need to test that the output of ckpt_export is the same whether I call ckpt_export or use the command line. What I would do is call the command line just to verify it exists and accept the parameters, but I would mock the actual rerun of the ckpt_export processes. Would you agree with that approach?
There was a problem hiding this comment.
I think that's fine, it would be more rigorous to do as the code originally did and check the output, but we're probably certain enough just checking the command works is fine.
| _, metadata, extra_files = load_net_with_metadata( | ||
| ts_file, more_extra_files=["inference.json", "def_args.json"] | ||
| ) | ||
| self.assertIn("schema", metadata) | ||
| self.assertIn("meta_file", json.loads(extra_files["def_args.json"])) | ||
| self.assertIn("network_def", json.loads(extra_files["inference.json"])) |
There was a problem hiding this comment.
This part of the tests, loading the TorchScript object back and inspecting properties, now isn't being done. I think this was useful to making sure the produced results were correct.
There was a problem hiding this comment.
Actually I see it in the second test now, but I wonder if we need two test methods now?
Fixes #8185
Description
Starts the work on test refactoring as described in issue #8185.
This is an initial pull request aimed at aligning the changes that I intend to make so the team can redirect me if I am going in the wrong direction.
I started by identifying the slowest tests in the stack which were not flagged as integration tests or downloads.
Starting with the slowest test on this criteria, I eliminated code duplications and rewritten the code for clarity.
I intended to mock some heavy functions and re-address the complete coverage by adding a separate integration test. However, just by replacing the
command_line_testwith a direct call of the functions, I was able to greatly reduce the running time without changing the test behaviour.I think this test should be renamed an integration test, but I await your reply.
Performance Before:
Performance after:
Types of changes
./runtests.sh --quick --unittests --disttests.