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

De-duplicate some unit test paramatrization #7800

Closed
wants to merge 3 commits into from

Conversation

dstansby
Copy link
Contributor

This removes a lot of duplication in parametrizing the unit testing, by extracting a common decorator into a new file. I had to move the tests to a new sub-folder to get this to work, I hope that's okay?

Copy link
Collaborator

@keewis keewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with #7799 I believe we can keep things within a single test file: create and append the unit info instance to unit_libs within a try-except block (so import errors cause only these tests to be skipped), and skip the entire file if unit_libs is empty.

In principle, though, this idea does sound good to me, as well.

@dstansby
Copy link
Contributor Author

Is keeping things in a single file a deliberate design choice? Personally from what I can see splitting up into separate files makes sense, given the original file is already so long.

@dstansby dstansby requested a review from keewis May 26, 2023 09:48
@max-sixty
Copy link
Collaborator

(I'm going through outstanding PRs)

This does look like a nice simplification. Should we try and resolve conflicts, decide on the single file, and merge?

@dstansby
Copy link
Contributor Author

I don't have time to finish this off now, but someone else is more than welcome to run with it 😄

@dstansby
Copy link
Contributor Author

Closing since I don't have time to work on this any more

@dstansby dstansby closed this Jan 11, 2024
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.

None yet

3 participants