Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use unittest's setUpClass instead of overriding __init__ #117

Merged
merged 2 commits into from
Jul 7, 2024

Conversation

davidwilby
Copy link
Collaborator

Hopefully this resolves #115 🤞

Whilst investigating the issue raised in #115 I noticed a number of issues on which the order/combination of tests run influenced the behaviour.

I can't say that I fully follow the reason behind the pollution between the different test modules but these changes seem to produce the desired behaviour as far as I can tell 🤷

UnitTest recommends using the class method setUpClass (similar to setUp but runs only once for all of the tests in the class)

Have run this locally and all tests pass, let's see what happens on GitHub actions...

Note that this branch doesn't include the fixes to the CI testing workflow in #116 so technically the tests on this branch will be running with the runner's OS python installation, not those managed by the python version matrix.

@tom-andersson
Copy link
Collaborator

Tyvm @davidwilby! CI looks happy, so let's go ahead and merge and (fingers crossed) once tests are passing on main we can assume this resolves #115.

Note: Reminder to run black just before committing (I've done it for you in this instance)

@tom-andersson tom-andersson merged commit 7157890 into main Jul 7, 2024
6 of 8 checks passed
@tom-andersson
Copy link
Collaborator

Yup, tests were failing just before merging this PR, and are now passing. Nice!

Perhaps import deepsensor.torch in test_model wasn't playing well with import deepsensor.tensorflow in test_active_learning, triggering backends to complain about mixed types. I don't fully understand why this test leakage would start becoming a problem seemingly "out of nowhere" though. Oh well, it's fixed now.

@tom-andersson
Copy link
Collaborator

@all-contributors please add @davidwilby for test

Copy link
Contributor

@tom-andersson

I've put up a pull request to add @davidwilby! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix failing tests due to PT -> TF type conversion (plum issue?)
2 participants