Replaced --forked in internal tests by utsavm9 · Pull Request #205 · jpmorganchase/nbcelltests

utsavm9 · 2020-09-23T05:45:39Z

Resolves #107:

pytest-xdist will now be installed on Windows
Replaced --forked with -n 2
Tests like test_basic_runWithReturn_pass and 4 others do not want to have the file __cell_coverage_test.py already created. I grouped all these 5 tests under a class TestCoverage so that these tests are run sequentially by a worker spawn by pytest-xdist
Removed special handling of --forked. Now, tests like test_state behave as if it's always FORKED=False.

Naming Choices:

Would you want to rename make pytest-forked to something else?
Azure test pytest --forked renames to pytest -n 2
Is the class name TestCoverage apt for grouping these tests? test_basic_runWithReturn_pass, test_basic_runWithReturn_fail, test_basic_runWithReport_pass, test_basic_runWithHTMLReturn_pass, test_basic_runWithHTMLReturn_fail

It is not apparent looking at the diff of test_test.py TestCoverage, but I have only added those five tests to the class and I have not changed those tests.

timkpaine · 2020-09-23T05:55:56Z

these are failing lint, run make fix for auto fix

ceball

This is great, thanks so much!

Thanks also for making your change so easy to review.

Would you want to rename make pytest-forked to something else?

Yes, I agree - see comment inline for the azure version.

We'll be happy to accept this PR, but before merging I want to discuss two aspects. One is covered in the comments (i.e. "distribution algorithm"); the other is:

Tests like test_basic_runWithReturn_pass and 4 others do not want to have the file __cell_coverage_test.py already created. I grouped all these 5 tests under a class TestCoverage so that these tests are run sequentially by a worker spawn by pytest-xdist

Please can you explain again why you are grouping these? I can see 3 possible reasons: (a) reduction of duplicated code, (b) grouping genuinely related tests together, i.e. in a "semantically meaningful way", (c) because otherwise the tests don't pass e.g. in the distributed case.

I can't quite tell which of those reasons (or maybe another?) applies here from your comment.

Is the class name TestCoverage apt for grouping these tests

Ideally I would like to group only if it makes real sense to do so, not if it's fixing some problem we could solve another way. So the answer to this one depends on the outcome of the discussion above.

utsavm9 · 2020-09-23T18:34:26Z

Please can you explain again why you are grouping these?

You guessed it right, without grouping the tests will not always pass. Those tests first check if __cell_coverage_test.py has not already been created and fail if it already exists. The file would be present if another of those 5 tests is running in another worker.

nbcelltests/nbcelltests/tests/test_test.py

Line 690 in aef8e32

generates = os.path.join(os.path.dirname(__file__), "__cell_coverage_test.py")

So, those 5 tests cannot be parallelized. One way to make pytest-xdist run those sequentially is to group them in a class and pass --dist=loadscope. From the docs:

--dist=loadscope: Tests are grouped by module for test functions and by class for test methods. Groups are distributed to available workers as whole units.

ceball · 2020-09-23T18:42:03Z

Quick comment from my phone...

I think those tests (or the underlying functions they test) should be fixed. E.g. they should use a unique temporary file.

We should either do that here in this PR, or open an issue to do it in the future. Up to you! (Off the top of my head I don't know if it's a big change or not.)

ceball

I do not think it is different from the invocation on Azure. I think it just callsmake testpy-distributed now which will come to Makefile and run the command with --dist=loadscope. Does it not?

Ok, great. (Likely I misread the diff :) )

utsavm9 · 2020-09-23T18:58:12Z

I am not sure at all why TestSkips is failing to setup once on Linux Python37. I am trying to duplicate that on my local machine.

Let's open another issue to make those tests independent. There are two files involved: __cell_coverage_test.py and _cell_coverage_test.py, so it will take a while and I can get back to it after some time.

ceball · 2020-09-23T20:02:19Z

I am not sure at all why TestSkips is failing to setup once on Linux Python37. I am trying to duplicate that on my local machine.

Looked like a timeout to me, probably just from azure. So I re-ran, and it passed.

(If it was down to some bad interaction between the tests that only shows up sometimes in the distributed case, we'll address it if we see it again in the future...)

utsavm9 · 2020-09-23T20:06:30Z

One last thing to ask: Would you like to be able to do make testpy-distributed N=8 along with make testpy-distributed? I personally have been using N=4 on my university's Linux server to test my changes as that was the fastest on that machine. See this: https://github.com/utsavm9/nbcelltests/commit/c934476b3cc8e89bc210c33c6bb7d7a0bab86bb5

Apart from that, I am all done with this PR!

ceball · 2020-09-23T20:22:53Z

Let's open another issue to make those tests independent. There are two files involved: __cell_coverage_test.py and _cell_coverage_test.py, so it will take a while and I can get back to it after some time.

Are you able to make the issue (extracting and summarizing our discussion here)? (No obligation; if not, I will do it.)

ceball · 2020-09-23T20:27:51Z

One last thing to ask: Would you like to be able to do make testpy-distributed N=8

Please could you make that as a separate PR, after this one? I am not at all against adding an option for varying n when running the tests (I think it's useful), but I don't own the Makefile so would want a teammate to ok it.

utsavm9 · 2020-09-23T20:32:54Z

OK, I will make another PR to master after this PR is merged. I can make the issue about those tests too. Thanks!

ceball

Just requested some comments (and ideally a new issue), otherwise this PR is fine with me. Thanks!

ceball

Thanks for all the follow-up work!

ceball · 2020-09-23T21:40:30Z

I'll merge once the tests pass (can't merge until then).

utsavm9 added 5 commits September 21, 2020 17:52

Install pytest-xdist on all platforms

15f82a1

--forked replaced with -n 2

f0384da

Grouped tests in class that need to be sequential

1606e27

Removed --forked and the handling for it

120330f

Corrected spelling

8efb3fa

Satisfy linter

aef8e32

ceball reviewed Sep 23, 2020

View reviewed changes

Comment thread .azure/dev-template.yml Outdated

Comment thread Makefile

Renamed to testpy-distributed

241745a

ceball reviewed Sep 23, 2020

View reviewed changes

ceball requested changes Sep 23, 2020

View reviewed changes

Comment thread nbcelltests/tests/test_test.py

utsavm9 added 2 commits September 23, 2020 14:04

Comments to explain --dist=loadscope

f81b1be

Added link to issue

2b2921c

ceball reviewed Sep 23, 2020

View reviewed changes

Comment thread Makefile Outdated

Link to issue

40639e2

ceball approved these changes Sep 23, 2020

View reviewed changes

ceball merged commit e5e1147 into jpmorganchase:master Sep 23, 2020

Uh oh!

Conversation

utsavm9 commented Sep 23, 2020

Uh oh!

timkpaine commented Sep 23, 2020

Uh oh!

ceball left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

utsavm9 commented Sep 23, 2020

Uh oh!

ceball commented Sep 23, 2020

Uh oh!

ceball left a comment

Choose a reason for hiding this comment

Uh oh!

utsavm9 commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ceball commented Sep 23, 2020

Uh oh!

utsavm9 commented Sep 23, 2020

Uh oh!

ceball commented Sep 23, 2020

Uh oh!

ceball commented Sep 23, 2020

Uh oh!

utsavm9 commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ceball left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ceball left a comment

Choose a reason for hiding this comment

Uh oh!

ceball commented Sep 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ceball left a comment •

edited

Loading

utsavm9 commented Sep 23, 2020 •

edited

Loading

utsavm9 commented Sep 23, 2020 •

edited

Loading