Mark tests as proprietary

[mivermeu]

This PR aims to resolve #543 by adding markers to any test that requires non-public data.

1. A new pytest marker "proprietary" was added and applied to relevant tests. If all tests in a TestCase class would be marked, the class itself was marked instead. If all tests in a module would be marked, the module as a whole was marked with pytestmark = pytest.mark.proprietary instead.
2. Proprietary test files that were loaded on module import were either moved to class methods or pytest fixtures.
3. CI/CD piplines were marked with the -m 'proprietary or not proprietary option.

[jrkerns]

Thanks for starting this. I think we can avoid a lot of code changes knowing that pytest runs all test by default, so doing -m 'proprietary or not proprietary' + the addopts is unnecessary to add everywhere. We run these tests all the time locally and in our CI/CD, so by defaulting to not proprietary the onus is on any RadMachine developer to remember to always be adding -m 'proprietary or not proprietary' which we will definitely forget 😉. You can alternatively add a mark only to the public, say, @pytest.mark.public, datasets which are pretty small right now (I think just the demos right now) to do pytest -m public. That would be just a few lines of code.

I appreciate the start here. One project I've never gotten around to was a public dataset site/archive. Something like the NIH Imaging Data Commons with a much smaller and focused scope on QA. Although I'd like to make a nice site with filtering, etc a pragmatic start would be a spreadsheet which could later be converted. Google forms could be used here for entering relevant information such as phantom type, etc. Anonymization would have to be thought about, although at the beginning we could simply place that on the uploader and later perform a best-effort anonymization and/or hash the uploader, clinic, etc. I can make a a stub for that and create a new issue to discuss that. I've been looking for an excuse to use Anvil. Integrating that with the public tests here would be quite nice!

[mivermeu]

Thanks for the feedback! I think you make a good point omitting the -m 'proprietary or not proprietary' and addopts changes. It's better to run into some failed tests due to access permissions than to think everything's alright because half of the tests didn't run at all.

I'd still advocate for a proprietary marker over a public one since it's the tests that use proprietary data that are in need of some attention. This way we'll have indicators of the test data that is still non-public. Besides, by pytest's count there are currently 4977 tests (impressive!), 3441 of which use non-public data and 1536 of which do not, so that would still be a lot of public markers.

I've added a line to the Contributing page of the documentation warning people of the proprietary test data. Feel free to change anything of course.

One project I've never gotten around to was a public dataset site/archive. Something like the NIH Imaging Data Commons with a much smaller and focused scope on QA.

Anvil looks like an appropriate tool for the job and I'd be interested to see a start of this project. I think this is a great idea!

[alanphys]

Is there any further progress with this? It's really a problem not being able to run the tests.

[jrkerns]

I have an Anvil app in the works for uploading machine QA data in a public setting. It still needs some hash checking and some instructions, but it's getting there. That would solve the data issue. Then, test cases have to be written for that data. We're spread quite thin on the team at the moment and pylinac has been on the back burner in favor of some refactors. But attention will come back after those refactor projects. We will also likely have some separate good news to share in the next quarter.

I'll merge this PR soon after I ensure everything works on the bitbucket CI/CD side.

[jrkerns]

Running the tests cause ~150 errors, mostly in test_image.py. AFAICT it's the fixtures alongside unittest. Although tests are run w/ pytest, a lot of tests are written in the unittest format. I have mixed feelings about pytest, but from my vague recollection fixtures caused issues which seem to also be the case here. I thought the point of putting fixtures in conftest.py was so that you didn't have to import them. It looks like you can use pytest_ignore_collect but that feels quite brittle. The marks are fine; we'll have to see if there's a better way to load data compatible w/ unittest.

A few failures for context.

[mivermeu]

I can see the issue with the fixtures. I've removed them and instead load the files in base classes, just as was already being done elsewhere in the code. This way importing the modules doesn't immediately result in a failure. Hopefully it works out better!

Interestingly the following failure remains on my side: "FAILED tests_basic/test_field_profile_analysis.py::FieldAnalysisTests::test_pdf_gets_generated - KeyError: 'X Metrics'" It looks like it's unrelated to the changes in this PR though.

[alanphys]

Thanks @mivermeu I've managed to run these on my side. In addition to the 'X Metrics' error I also get: FAILED tests_basic/test_winstonlutz.py::TestAlignPoints3D::test_perfect - AssertionError: 21.105869846768247 != 0 within 7 places (21.105869846768247 difference)

But this may be due to the platform (Fedora) I'm running on.

Unfortunately the tests I really need, core/test_image.py, appear to be all proprietary.

Regards Alan

[mivermeu]

Thanks for keeping this PR moving @alanphys! I took a look at that test and unfortunately it completes fine for me. I see that this test compares a set of points [(0, 0, 0), (1, 1, 1)] to itself, so I don't see how that would produce 21.105869846768247 in x, y, z or yaw. Which of the assertions in the test function fails for you?

@jrkerns Any thoughts on this? Also, is there anything that you'd like to see changed before this PR can be merged?

[purepani]

I have all the tests passing on my end, except for FieldAnalysisTests.test_pdf-gets_generated which generates a key error on 'X Metrics', though obviously the merge conflicts need to be fixed.