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

Wrap unittest method test in TestSuite #3295

Merged
merged 4 commits into from
Dec 11, 2018
Merged

Wrap unittest method test in TestSuite #3295

merged 4 commits into from
Dec 11, 2018

Conversation

alexander-yu
Copy link

@alexander-yu alexander-yu commented Nov 10, 2018

For #3252

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Has a news entry file (remember to thank yourself!)
  • Unit tests & system/integration tests are added/updated
  • Test plan is updated as appropriate
  • package-lock.json has been regenerated by running npm install (if dependencies have changed)

This PR addresses something I noticed in #3252, where clicking Run Test for a single unittest method (when choosing unittest as the test framework) leads to setUpClass not being run, even though it does occur when running Run Test for the entire class.

Looking through the test launcher, it appears that if we're running a single test method, then we actually end up running a bare TestCase object, whose run method (which is called by the unittest test runner) only calls setUp. Wrapping the TestCase object with a TestSuite class ensures that setUpClass is called.

Copy link

@d3r3kk d3r3kk left a comment

Choose a reason for hiding this comment

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

This is a great change, I think we should ultimately accept it, thanks!

However, could you add a test plan for this adjustment to the Python code? We don't have unit or system tests set up (yet) for Python files in our extension - something I hope to rectify in the next quarter.

A test suite that would show the behaviour without your change would be one where tests fail if the suite initialization method is not called first. Please add this to .github/test_plan.md.

Also, please add a news entry for your PR (remembering to thank yourself) and I will accept.

Again, thanks!

@alexander-yu
Copy link
Author

Sure thing! I'll see what I can do.

@msftclas
Copy link

msftclas commented Nov 25, 2018

CLA assistant check
All CLA requirements met.

@alexander-yu
Copy link
Author

@d3r3kk modified the test plan + added news entry; also merged the newest upstream changes.

@d3r3kk
Copy link

d3r3kk commented Dec 11, 2018

Thanks for finishing up @alexander-yu! I will take this up with the team tomorrow and see if we want to include it for the next release after our mid-December.

@d3r3kk d3r3kk merged commit a29ce05 into microsoft:master Dec 11, 2018
@d3r3kk
Copy link

d3r3kk commented Dec 11, 2018

🎆

We've merged the change, will be included in the upcoming release for Dec.

@alexander-yu alexander-yu deleted the fix-unittest-run-method branch December 12, 2018 00:48
@lock lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants