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

Update deprecated pytest api #43

Closed
wants to merge 5 commits into from
Closed

Conversation

tonyfast
Copy link

@tonyfast tonyfast commented Sep 8, 2020

Pytest recently deprecated how modules are instantiated modules that causes ci failures. I ran into this issue on a different project https://github.com/deathbeds/importnb/pull/103/files

This pr adds logic for the newest api.

nbsmoke/__init__.py Outdated Show resolved Hide resolved
@ceball
Copy link
Member

ceball commented Sep 8, 2020

In #42 I'd been trying to remove all this code - now I wish I'd finished that 😂

@ceball
Copy link
Member

ceball commented Sep 8, 2020

I guess users aren't seeing a problem yet as it's just a deprecation warning? I.e. this problem is currently just showing up in the test suite because nbsmoke's test suite is being strict about checking for unexpected warnings (because it's testing for presence/absence of some warnings deliberately).

If you encountered as a user, on the other hand, then this seems like something we should fix as soon as possible...

@ceball
Copy link
Member

ceball commented Sep 8, 2020

Either way, thanks!

@tonyfast
Copy link
Author

tonyfast commented Sep 8, 2020

test_lint_bad_silenced_with_noqa seems to be failing consistently. any thoughts on what could be causing that lint error.

@ceball
Copy link
Member

ceball commented Sep 8, 2020

Just on a phone here, but when I look at travis, I see:

==================================== ERRORS ====================================
______________________ ERROR collecting testing123.ipynb _______________________
/home/travis/build/pyviz-dev/nbsmoke/.tox/py37/lib/python3.7/site-packages/nbsmoke/__init__.py:143: in collect
    yield self._dowhat(str(self.fspath), self)
/home/travis/build/pyviz-dev/nbsmoke/.tox/py37/lib/python3.7/site-packages/_pytest/nodes.py:95: in __call__
    warnings.warn(NODE_USE_FROM_PARENT.format(name=self.__name__), stacklevel=2)
E   pytest.PytestDeprecationWarning: Direct construction of LintNb has been deprecated, please use LintNb.from_parent.
E   See https://docs.pytest.org/en/stable/deprecations.html#node-construction-changed-to-node-from-parent for more details.
=========================== short test summary info ============================
ERROR testing123.ipynb - pytest.PytestDeprecationWarning: Direct construction...
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=============================== 1 error in 0.19s ===============================
=========================== short test summary info ============================
FAILED tests/test_lint.py::test_lint_bad_silenced_with_noqa - assert <ExitCod...
======================== 1 failed, 37 passed in 10.73s =========================

i.e. there's another similar api change to make?

@ceball
Copy link
Member

ceball commented Sep 8, 2020

https://github.com/pyviz-dev/nbsmoke/blob/ba28c86938d8c2e48e07b98208f405aef811e248/nbsmoke/__init__.py#L137-L143

dowhat being RunNb or LintNb or VerifyNb - subclass of pytest.Item - which, like the IPyNbFile(pytest.File) you already fixed is a pytest Node

maybe?

I would need to open all these files up at the same time and consider it to know for sure what to do...

@@ -140,7 +140,10 @@ def __init__(self, fspath, parent=None, config=None, session=None, dowhat=RunNb)
super(IPyNbFile,self).__init__(fspath, parent=parent, config=None, session=None)

def collect(self):
yield self._dowhat(str(self.fspath), self)
if hasattr(self._dowhat, "from_parent"):
yield self._dowhat.from_parent(self, fspath=self.fspath)
Copy link
Member

@ceball ceball Sep 9, 2020

Choose a reason for hiding this comment

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

Maybe

Suggested change
yield self._dowhat.from_parent(self, fspath=self.fspath)
yield self._dowhat.from_parent(self, name=str(self.fspath))

to address the error in travis:

==================================== ERRORS ====================================
______________________ ERROR collecting testing123.ipynb _______________________
/home/travis/build/pyviz-dev/nbsmoke/.tox/py37/lib/python3.7/site-packages/nbsmoke/__init__.py:144: in collect
    yield self._dowhat.from_parent(self, fspath=self.fspath)
/home/travis/build/pyviz-dev/nbsmoke/.tox/py37/lib/python3.7/site-packages/_pytest/nodes.py:195: in from_parent
    return cls._create(parent=parent, **kw)
/home/travis/build/pyviz-dev/nbsmoke/.tox/py37/lib/python3.7/site-packages/_pytest/nodes.py:99: in _create
    return super().__call__(*k, **kw)
E   TypeError: __init__() got an unexpected keyword argument 'fspath'
=========================== short test summary info ============================
ERROR testing123.ipynb - TypeError: __init__() got an unexpected keyword argu...
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=============================== 1 error in 0.29s ===============================

It's sure confusing :( str(fspath) is being used as name.

(Item requires name: https://github.com/pytest-dev/pytest/blob/885d96948441938877d6222494c87c64a1b0f476/src/_pytest/nodes.py#L552)

(The from_parent() "indirection got introduced in order to enable removing the fragile logic from the node constructors" - you can never have enough indirection 😂)

If anyone ever understood all of this at any point, they could address #29...although again, ultimately I hope to delete a lot of this project...

Copy link
Member

Choose a reason for hiding this comment

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

This suggestion is now hidden/showing as "outdated" for me in github, but I think it's the right thing to do: see #44 where it worked (and where after updating for pytest land, I discover I also need to update for jupyter land...).

Copy link
Member

@ceball ceball left a comment

Choose a reason for hiding this comment

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

Once the tests pass, should merge. Then we should release a new version. Thanks!!

ceball added a commit that referenced this pull request Sep 10, 2020
@ceball ceball mentioned this pull request Sep 10, 2020
@ceball
Copy link
Member

ceball commented Sep 10, 2020

Thanks for getting this started! Superseded by #44

@ceball ceball closed this Sep 10, 2020
ceball added a commit that referenced this pull request Sep 10, 2020
* Apply my suggestions from #43.
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.

2 participants