lvgig / tubular

Python package implementing transformers for pre processing steps for machine learning.
https://tubular.readthedocs.io/en/latest/index.html
BSD 3-Clause "New" or "Revised" License
37 stars 14 forks source link

Double check test inheritances #255

Closed limlam96 closed 2 weeks ago

limlam96 commented 3 weeks ago

What?

Spotted that tests from test_BaseMappingTransformerMixin.py are not imported anywhere, would expect that these would be inherited by relevant test classes in mixin-like way so that these classes also inherit all test cases.

There is this comment at the top of the file: '# Note there are no tests that need inheriting from this file as the only difference is an expected transform output'

So it may be that the test cases are already covered and this is not a bug, but think I would still lean towards refactoring to have tests mirror mixin structure.

Task is to investigate this and make any changes necessary. If this is not a bug, we can proceed with a release and think about refactoring at later date.

Related follow up task will be to double check other test inheritances to ensure that other classes have correct test suite.

Why?

Ensure tests are being run as expected

How?

Start by investigating testing setup for MappingTransformer to investigate if BaseMappingTransformerMixin functionality is covered.

After resolving MappingTransformer, would do a quick scan of other files to check for similar missed mixin issues.

davidhopkinson26 commented 3 weeks ago

This is intentional. The tests here are intended for specifically unit testing the mixin methods and were not intended to be inherited in other test classes.

the other tests are not exactly duplicated from GeenricTransformTests as they set _

All in all no changes required as far as I can tell so this shouldn't stop a release.

Weirdness all comes from having both BaseMappingTransformer and BaseMappingTransformerMixin classes which is quite confusing and should probably be a target for future refactors.

EDIT: had a bit more of a play and corrected a couple of the points above. no recommended changes to tests.

davidhopkinson26 commented 2 weeks ago

closing this issue after above investigation and lack of further activity.