-
Notifications
You must be signed in to change notification settings - Fork 233
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
Workaround cpu detection on Travis CI #317
Conversation
This branch used in diofant/diofant#664 |
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.
Hi @skirpichev,
Thanks for the implementation.
We do need a test though, mostly to avoid future regressions.
Could you also refactor the code inside the if s == "auto":
into a separate function, like auto_detect_cpus()
? You can more easily test the outcome of that function. We already have some tests in place for that functionality, you can use them as basis for your test.
Thanks again.
Ok, done. Not sure, maybe last commit should be squashed with first. Let me know if I should do anything. BTW, tests in diofant/diofant#664 pass too. |
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.
Thanks @skirpichev, LGTM!
As soon as we merge this I will prepare a new release.
@RonnyPfannschmidt anything you want to take a look here?
Improved the test a bit to always test the Travis fallback. 👍 |
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.
looks ok, as followup we could take a look at moving the auto-detection tests from acceptance tests to lower integration tests
…test-xdist#317). And format fixes.
Closes #316
Not sure if this worth test.