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

Change parameterized testing system to be compatible with unittest #712

Merged
merged 2 commits into from Jun 11, 2020

Conversation

mthrok
Copy link
Collaborator

@mthrok mthrok commented Jun 10, 2020

Summary: The previous implementation of parameterized testing worked by modifying test.common_utils inplace. This doesn't work in general because unittest's contract with test modules is such that it must be able to load the module and run the test itself. Because the previous implementation needed to load the module and modify it, it is incompatible.

Reviewed By: mthrok

Differential Revision: D21964676

Summary: The previous implementation of parameterized testing worked by modifying test.common_utils inplace.  This doesn't work in general because unittest's contract with test modules is such that it must be able to load the module and run the test itself.  Because the previous implementation needed to load the module and modify it, it is incompatible.

Reviewed By: mthrok

Differential Revision: D21964676
@mthrok mthrok requested a review from vincentqb June 10, 2020 17:20
@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

Merging #712 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #712   +/-   ##
=======================================
  Coverage   89.23%   89.23%           
=======================================
  Files          26       26           
  Lines        2350     2350           
=======================================
  Hits         2097     2097           
  Misses        253      253           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c82a7f9...54a2029. Read the comment docs.

@vincentqb
Copy link
Contributor

Summary: The previous implementation of parameterized testing worked by modifying test.common_utils inplace. This doesn't work in general because unittest's contract with test modules is such that it must be able to load the module and run the test itself. Because the previous implementation needed to load the module and modify it, it is incompatible.

Can you give a snippet of the error we were getting on the internal side?

Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

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

Based on offline discussion, this LGTM. I agree modules should not be modified by other modules.

@vincentqb
Copy link
Contributor

vincentqb commented Jun 11, 2020

Are there other cases behaving like this that use define_test_suites and globals?

@mthrok
Copy link
Collaborator Author

mthrok commented Jun 11, 2020

Are there other cases behaving like this that use define_test_suites and globals?

Ya, I am working to remove them now.

@mthrok mthrok merged commit d272448 into pytorch:master Jun 11, 2020
@mthrok mthrok deleted the fix-test-parameterization branch June 11, 2020 18:05
@mthrok
Copy link
Collaborator Author

mthrok commented Jun 11, 2020

Thanks

mpc001 pushed a commit to mpc001/audio that referenced this pull request Aug 4, 2023
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