-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
TST: Call tests just once with --dist=loadscope #28531
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
TST: Call tests just once with --dist=loadscope #28531
Conversation
If this finally works and can be merged, next step is to reimplement #26932. It failed before in the second run of the tests, but after this is merged, we'll have only one run, and it will work. That will make the clipboard tests run again in the CI. |
There are some tests failing, I guess they use the same shared resources, but are not in the same scope, but the timings look ok. Comparing times with #28542, those are the results:
I'll have a look at |
Looks like in I think we'll have to detect the file(s) that are taking longer in See updated timings:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly simpler - this lgtm so I'd say merge if no other feedback and you have follow ups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am weary of this let me look
Would be nice to get this merged somehow soon, so we can restore the clipboard tests. But this PR is a bit tricky and somehow risky, so let's review well, and re-run the CI couple of times before merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of this change, but I just don't want unexplained failures like we had before when we had resource contention. can you provide some insight on the question below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok comment on deps; also should followup with a PR removing the marks themselves (single,multiple); let's not crowd this one. pls rebase as well. ping on green.
ok one more rebase; ping on green. |
Having this error in Travis:
Not sure why this is failing here. Shouldn't be caused by the changes here. Has anyone seen this error before? |
@datapythonista are we actually running the x clip tests currently? i thought was part of the issue |
I thought we moved those tests to azure, where they were skipped. Not sure if the failing build was added after that. I'm in my phone, can't check in detail now. |
@datapythonista still working on this? |
Yes, still need to figure out what's the problem with the failing tests, but want to get this merged. |
can you rebase when you have some time |
it looks like the 3.6 coverage job is taking 50% longer than on master. can you confirm? (45 min) maybe rebase and see if this repeats. |
thanks @datapythonista let's monitor the CI going forward for any odd failures |
I created #29671 to speed up the builds. |
great in practice we are already splitting test files if they r too big (in loc) |
Potentially related seeing this on travis 36 builds
|
Another try to what was tried in #26949. Before this PR the tests are called twice, once in a core for the tests that affect shared results, and once in parallel for the rest.
This PR makes a single call, and tests in the same scope (class or module) are granted to run in the same core, so no shared data problems should happen.
The tests being very slow was possibly caused by the proxy env variables, and not
--dist=loadscope
. But please check how long tests took before merging.