[ORCA-284] External Test Testing & [ORCA-287] OmeXmlSchemaTest Bug fix

[BWMac]

ORCA-284 Problem:

Currently, there is no way to run external tests without using nf-dcqc, and therefore there is no way to easily test them to ensure that they are evaluating files as intended.

ORCA-284 Solution:

Implement Docker-enabled tests that can run the exact commands that nf-dcqc currently does via a local or GitHub Actions Docker environment.

Of note in this implementation:

1. docker python package was added as a testing dependency
2. BioFormatsInfoTest was updated to specify the OMETiff file format in its CLI command. The test was producing inconsistent results in local testing without this specified. This change has been tested in nf-dcqc and yielded the same consistent results as before.
   • Currently, this test is only used in the OmeTiffSuite so this should not cause problems unless
     it is added to other/new suites.
3. test_tests.py has been converted into test_external_tests.py (I should have just created the new files and deleted the old, the diff is a little weird), and internal test unit tests have been split off into test_internal_tests.py.
4. In order to support Docker-enabled testing, a new class DockerExecutor was introduced to test_external_tests.py. This class takes the docker image string and command from the Process object generated for external tests, in addition to the path to a testing file, and creates a DockerExecutor which can run external test commands by invoking its execute method.
   • A couple of tweaks were needed to standardize command execution across containers:
     • The format_command_for_sh method escapes special characters in the
       command and prefaces it with sh -c to ensure that commands are executed in a similar
       fashion across Docker containers
     • working_dir was set to "/" in client.containers.run to ensure that files are able to be
       accessed once mounted, regardless of WORKDIR configurations in specific images.
5. While running these new tests in GitHub actions, they worked in the ubuntu runner immediately, but macos proved trickier. I did some research and long story short GitHub did add support to easily run Docker in the macos runner in this PR, but it has been reported recently that this has stopped working due to changes in dependencies. As an interim solution, I implemented a simple wrapper that causes Docker-enabled tests to be skipped when they are run in a non-Linux environment, rather than adding more complex changes to our CI workflow.

ORCA-287 Problem:

OmeXmlSchemaTest always returns an exit code of 0, even when a file fails.

ORCA-287 Solution:

The results of the bftools/xmlvalid command are actually found in its stdout, so instead of evaluating directly on the first exitcode, we pipe the output into a grep command and look for the substring that indicates success: "No validation errors found.". This way, the pass_code for the test can remain "0", but when the file fails the test it can produce the expected "1". This change has been tested in nf-dcqc and with local Docker execution.

[swarmia]

✅  Linked to Bug ORCA-287 · OmeXmlSchemaTest always returns an exit code of 0

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (68f9564) 100.00% compared to head (52e22bb) 100.00%.

Additional details and impacted files

@@            Coverage Diff            @@
##              main       #50   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines         1184      1184           
  Branches       192       192           
=========================================
  Hits          1184      1184           

Files	Coverage Δ
src/dcqc/tests/bioformats_info_test.py	100.00% <ø> (ø)
src/dcqc/tests/ome_xml_schema_test.py	100.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[BryanFauble]

(No changed needed, but I wanted to share some resources with you).

This article gives a great overview of Behavior Driven Development (BDD) topics that can be applied to testing: https://pythontest.com/strategy/given-when-then-2/

BDD is also heavily used by automation engineers when writing automated end-to-end testing in a number of frameworks, so it's likely you'll run into this eventually.

I find thinking in a GIVEN-WHEN-THEN structure for my tests gives me clearer focus that I am testing a singular thing, I know what my assumptions are, and I know the actions I am taking.

I started to use this structure in the Synapse Python Client:

1. Sage-Bionetworks/synapsePythonClient@0d1721f#diff-5056ccf1a22d1e69ce5aeea47808fc00ebe8173df1b70a9eb6cd02d2d38cad51R505
2. Sage-Bionetworks/synapsePythonClient@c6223b5#diff-eb75db4e0b13e5e4845036998dcf3c8c8c5509fb0560b21f5e7d1d21ec448d7bR356

[BWMac]

This is great food for thought and I will look to incorporate these concepts in future work!

[BryanFauble]

nit: I could see these static strings through the test being an enum of the possible valid values you could have: https://docs.python.org/3/library/enum.html

[BWMac]

I agree. I already created a ticket for reworking the test file names because I find them to be confusing. I can explore this option as part of that task.

[BryanFauble]

Food for thought (feel free to take it or leave it):
You did a great job splitting the tests up into the various python scripts - One area that could be improved is to have 1 more level of organization within the script by grouping like tests together. For example:

class TestBioformats:
    def test_....
    def test_....

class TestTiffDateTimeTest:
    def test_...

Further reading: https://docs.pytest.org/en/7.1.x/getting-started.html#group-multiple-tests-in-a-class

[BWMac]

I did consider this and have used test classes in the past. The reason I did not do it in this ticket is that the internal test unit tests don't have this structure and I wanted to keep the testing organization consistent while staying within the external test scope.

I propose a separate ticket that handles this refactoring of internal and external test unit tests into classes. I think this is a great organizational addition that will make contributing new tests and their unit tests to this repo simpler.

Edit: I created this ticket to track this

[BryanFauble]

Just a few comments I left, otherwise LGTM!

[thomasyu888]

🔥 LGTM!