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

Casting fixture parameter to list at the beginning of parameter parsing. #5950

Merged

Conversation

Sup3rGeo
Copy link
Member

Closes #5946

  • Target the master branch for bug fixes, documentation updates and trivial changes.
  • (NON APPLICABLE) Target the features branch for new features, improvements, and removals/deprecations.
  • (NON APPLICABLE) Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
  • Add yourself to AUTHORS in alphabetical order;

src/_pytest/fixtures.py Outdated Show resolved Hide resolved
testing/python/fixtures.py Show resolved Hide resolved
@nicoddemus
Copy link
Member

Looks great, thanks @Sup3rGeo!

Co-Authored-By: Bruno Oliveira <nicoddemus@gmail.com>
@Sup3rGeo
Copy link
Member Author

Sup3rGeo commented Oct 12, 2019

It seems that because we now always have numpy in the testing environments, the part that return False in this function never gets executed/tested:

def _is_numpy_array(obj):
"""
Return true if the given object is a numpy array. Make a special effort to
avoid importing numpy unless it's really necessary.
"""
import sys
np = sys.modules.get("numpy")
if np is not None:
return isinstance(obj, np.ndarray)
return False

What to do about it?

EDIT: I guess parametrize the approx tests with and without the numpy module? maybe by monkeypatching sys.modules.get to return None on those tests?

@nicoddemus
Copy link
Member

TBH I don't think we need to bother testing that, adding # pragma: no cover to the return False should be enough.

@nicoddemus nicoddemus merged commit 3322c1e into pytest-dev:master Oct 12, 2019
@nicoddemus
Copy link
Member

Thanks for the PR and reference! 👍

@@ -28,6 +28,7 @@ def main():
"mock",
"nose",
"requests",
"numpy",
Copy link
Contributor

Choose a reason for hiding this comment

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

@nicoddemus
Adding numpy to all builds make the numpy tox env obsolete, doesn't it?
I'd rather not have it installed always, but in case this is kept it should clean up the env, jobs, and conditional skips we have for it.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, thanks for catching that.

Practically I think it makes sense to install an additional dependency in order to have less jobs, as an extra dependency is usually much quicker to install than starting up an entire new job, which potentionally might run all tests again.

I will follow up with a fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just started with it already myself.

Copy link
Contributor

@blueyed blueyed Oct 16, 2019

Choose a reason for hiding this comment

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

I wanted to keep numpy optional, but what you've said sounds different?
Keep in mind that numpy might be very slow to install without a wheel.

=> #5977

Copy link
Member

Choose a reason for hiding this comment

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

You are right.

Let's remove numpy from extras and use importorskip on the test.

I have the diff ready:

diff --git a/setup.py b/setup.py
index be38857b0..dcf63f6fd 100644
--- a/setup.py
+++ b/setup.py
@@ -28,7 +28,6 @@ def main():
                 "mock",
                 "nose",
                 "requests",
-                "numpy",
                 "xmlschema",
             ]
         },
diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py
index 4fcab0245..9ef7f0245 100644
--- a/testing/python/fixtures.py
+++ b/testing/python/fixtures.py
@@ -4190,6 +4190,7 @@ def test_indirect_fixture_does_not_break_scope(testdir):


 def test_fixture_parametrization_nparray(testdir):
+    pytest.importorskip("numpy")
     testdir.makepyfile(
         """
         from numpy import linspace

blueyed added a commit to blueyed/pytest that referenced this pull request Oct 16, 2019
MarcoGorelli pushed a commit to MarcoGorelli/pytest that referenced this pull request Oct 18, 2019
MarcoGorelli pushed a commit to MarcoGorelli/pytest that referenced this pull request Oct 18, 2019
Add link to technical aspects issue to the py27-py34 docs

Update pdb++ link (moved to GitHub)

Add pudb to project examples

Casting fixture parameter to list at the beginning of parameter parsing.

Added changelog file.

Fixed linting.

Always creating list for consistency.

Co-Authored-By: Bruno Oliveira <nicoddemus@gmail.com>

Workaround curl bug which makes retries of fetching codecov.io/bash not work

Port CHANGELOG from 4.6.6 release

changelog: pytest-dev#5523 was fixed in 5.0.1 already

Ref: pytest-dev#5952 (comment)

doc: caplog: add caplog.messages

Add missing version added/changed markers to docs

Notice some features since 5.0 were not being properly
marked in which version they have been added/changed.

tests: keep numpy being optional

Ref: pytest-dev#5950 (comment)

ci: Travis: move py37-pexpect to another job

It does not have to run all tests again by itself.

Remove redundant mention from 5.2.0 release notes.

Fix plurality mismatch for 'warnings' and 'error' in terminal summary

Make sure plurality match is respected in rest of codebase

Fix collection message

Add changelog entry

Update tests

Change keys so by default they're plural and rename _match_plurality to _make_plural

Fix rebase

Fix rebase

Fix rebase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fixture parametrization breaks if parameter is a numpy array
3 participants