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.

Sign up for GitHub

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

Jump to bottom

skip numpy and pandas tests if not importable #4563

Merged
merged 4 commits into from
Mar 24, 2021
Merged

skip numpy and pandas tests if not importable #4563

merged 4 commits into from
Mar 24, 2021

Conversation

bnavigator
Copy link
Contributor

@bnavigator bnavigator commented Mar 7, 2021 •
edited
Loading

@bnavigator bnavigator mentioned this pull request Mar 8, 2021
Closed
Base automatically changed from master to main March 8, 2021 19:04
jrbourbeau
jrbourbeau reviewed Mar 18, 2021
View reviewed changes
Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks for handling this @bnavigator!

distributed/protocol/tests/test_serialize.py Outdated
import pytest
from tlz import identity

np = pytest.importorskip("numpy")
Copy link
Member

Choose a reason for hiding this comment

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

Most tests in this module don't use NumPy and it'd be good to run them even when NumPy isn't installed. Could you isolate the importorskip behavior specifically to tests which need NumPy like you've done in some other test modules?

Copy link
Contributor Author

@bnavigator bnavigator Mar 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Not possible, because numpy is used at parametrization of tests, i.e. during test collection of the module.

If done right, it's possible. Done in c0552f9

@jakirkham
Copy link
Member

Also when you have a chance, could you please merge dask/main into your branch and push that here. The CI configuration used in the PR is a little stale and causing failures

@bnavigator
Copy link
Contributor Author

Rebased.

bnavigator
bnavigator commented Mar 23, 2021
View reviewed changes
Copy link
Contributor Author

@bnavigator bnavigator left a comment

Choose a reason for hiding this comment

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

More black errors

distributed/protocol/tests/test_serialize.py Outdated Show resolved Hide resolved
distributed/protocol/tests/test_serialize.py Outdated Show resolved Hide resolved
@bnavigator
Copy link
Contributor Author

Aw c'mon! Does the CI ever pass?

@jrbourbeau jrbourbeau mentioned this pull request Mar 24, 2021
Open
jrbourbeau
jrbourbeau approved these changes Mar 24, 2021
View reviewed changes
Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @bnavigator!

Here is our current list of flaky tests. Any help you're able to provide towards improving them is welcome.

@jrbourbeau jrbourbeau merged commit 89b70ac into dask:main Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrbourbeau jrbourbeau jrbourbeau approved these changes

@jakirkham jakirkham jakirkham approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Some tests require numpy and fail if not present
3 participants
@bnavigator @jakirkham @jrbourbeau