Remove pytest11 entrypoint and plugin, require tornado 6.1, remove asyncio patch, CI work by bollwyvl · Pull Request #339 · jupyter-server/jupyter_server

bollwyvl · 2020-11-13T05:23:54Z

fixes pytest_tornasync becomes hard dependency if pytest is installed #337
removes the entrypoint
updates some metadata in the example

kevin-bates · 2020-11-13T16:44:07Z

tests/conftest.py

@@ -1 +1 @@
-pytest_plugins = ['pytest_jupyter_server']
+pytest_plugins = ['jupyter_server.pytest_plugin']


I guess I'm confused on when this is necessary. Given these tests will depend on pytest-jupyter's pytest_jupytercore and pytest_jupyterserver plugins, do consumers of those fixtures (including jupyter_server) require a pytest_plugins statement or is the extra-requires [test] dependency on pytest-jupyter sufficient?

In working with #335, it doesn't seem like this entry (referencing the now external plugins) makes any difference.

If the pytest11 entry point in this repo is removed, there would be no particular way for pytest to know that it should use it, it wouldn't be imported, and the issue wouldn't occur for folk that have packages installed that try to actually .load their entrypoints.

Of the seven ways a plugin can be found, if it a test dependency doesn't use number 4, i'm a big fan of being explicit in conftest.py (number 7) or command line (number 1), but checked in as checked in inside a file that ends up in the distribution, e.g. pyproject.toml, or setup.cfg rather than rely on env vars, switches, etc.

do consumers of those fixtures (including jupyter_server) require a pytest_plugins statement

Once it's an explicit, real dependency, on a dedicated, test-time package, then everything is cool, and no pytest_plugin variable will be required, as number 4 will take care of it.

So generally: you'd never want anything referenced by Other People's Entry Points (what you might not control) to be behind an extra... depending on how they are used (e.g. have to be actually imported).

The whole prefix thing from #335? Yeah, well, that's just what a test regume is buying into with magic named functions. I'm not a huge fan, as the pattern (which, luckily, seems to be rather unique) doesn't play nice-nice with other tools (e.g. mypy, etc). But it's the only game in town, aside from the few little ecosystems where they felt they had to home-roll their own thing... noflo, robotframework, etc. have extremely unpythonic things they do, in the name of specific purposes.

bollwyvl · 2020-11-13T23:01:31Z

examples/simple/setup.py

        'jinja2',
    ],
+    extras_require = {
+        'test': ['pytest-jupyter'],


not strictly required, but if the example is supposed to be illustrative...

bollwyvl · 2020-11-13T23:01:46Z

examples/simple/setup.py

    description = 'Jupyter Server Example',
    long_description = open('README.md').read(),
-    python_requires = '>=3.5',
+    python_requires = '>=3.6',


I guess I'm not following the skull & crossbones commentary here, sorry about that. The change seems fine.

Just meant 3.5 is EOL!

kevin-bates · 2020-11-13T23:19:16Z

Thank you for your helpful responses and guidance on this Nick - it's much appreciated.

I missed the examples test, as you're now discovering - sorry about that. Looks like it's just the server_config and fetch references that require fp_ prefixes.

One of the commits in #338 I was interested in was: 288d579.
I've been finding that test_get_spec has been failing on my dev system and I'm curious if that failure is addressed by your commit? If so, are you able to include that here?

bollwyvl · 2020-11-14T01:42:15Z

Re: 288d579 yeah, MANIFEST.in probably needs some gardening. It's yet another thing that doesn't really work the way anyone expects it would.

The only way to be sure on this particular gotcha:

have a test that exercises every file expected to appear
build the whl/sdist, install it, and test that, not whatever happens to be in the repo
- tox actually does this by default, and a lot else... may be worth considering...

bollwyvl · 2020-11-14T18:26:16Z

windows. ALWAYS WINDOWS. i think we have a winner here: smells like a ProactorLoop thing. Somebody may need to kill that job (don't think I can)...

bollwyvl · 2020-11-14T18:55:01Z

Alright: codecov aside (didn't dig into what it was comparing against) this is ready for more serious review. Some of the changes to the test_file stuff could be reverted, but the parameterization is good for debuggability (e.g. what else would have broken, had the first thing tested not broken).

kevin-bates

Thanks for the great work here Nick - it's much appreciated. I just had the few comments.

Regarding the empty tests/conftest.py file - is that necessary?

kevin-bates · 2020-11-15T16:30:36Z

jupyter_server/serverapp.py

-                    # fallback to the pre-3.8 default of Selector
-                    asyncio.set_event_loop_policy(WindowsSelectorEventLoopPolicy())
+        """no longer needed with tornado 6.1"""
+        warnings.warn(


Should this remain behind a windows-3.8 check or be removed altogether? I understand its purpose, but now folks not on Windows-py3.8 systems will get this warning when it never applied to them in the first place.

removed entirely... but while hidden behind a "private" member, I wouldn't rule out people having used it to avoid The Condition. Could just be removed immediately, but a release with a warning seems reasonable vs Your Stuff Breaks Now.

Right on. I missed that the call server makes has been removed. This makes perfect sense - thanks.

kevin-bates · 2020-11-15T16:31:00Z

MANIFEST.in


 # include everything in package_data
-include jupyter_server/**/*
+recursive-include jupyter_server *


Good catch - thanks.

kevin-bates · 2020-11-15T16:37:42Z

examples/simple/setup.py

    description = 'Jupyter Server Example',
    long_description = open('README.md').read(),
-    python_requires = '>=3.5',
+    python_requires = '>=3.6',


I guess I'm not following the skull & crossbones commentary here, sorry about that. The change seems fine.

kevin-bates · 2020-11-15T16:40:45Z

tests/test_files.py

+
+async def fetch_expect_404(jp_fetch, *path_parts):
+    with pytest.raises(tornado.httpclient.HTTPClientError) as e:
+        r = await jp_fetch('files', *path_parts, method='GET')


This is a nice update - thank you.

Would you mind removing the unused variable?

fixed in da0fef5

bollwyvl · 2020-11-15T18:51:07Z

Hate that codecov ❌ ... trying to fix on this PR seems out of scope, though 😢 Otherwise back to green, review issues about addressed, hopefully...

kevin-bates · 2020-11-15T21:34:36Z

jupyter_server/serverapp.py

+    assert tornado.version_info >= MIN_TORNADO
+except (ImportError, AttributeError, AssertionError) as e:  # pragma: no cover
+    raise ImportError(
+        _("The Jupyter Server requires tornado >=%s", ".".join(MIN_TORNADO))


The version info isn't getting formatted into the string prior to its translation. Do the tuple elements also need to be strings for the join to work?

good point! Fixed in e09ba05

kevin-bates · 2020-11-15T21:34:44Z

jupyter_server/serverapp.py

-                    # fallback to the pre-3.8 default of Selector
-                    asyncio.set_event_loop_policy(WindowsSelectorEventLoopPolicy())
+        """no longer needed with tornado 6.1"""
+        warnings.warn(


Right on. I missed that the call server makes has been removed. This makes perfect sense - thanks.

kevin-bates · 2020-11-16T17:03:00Z

jupyter_server/serverapp.py

+    assert tornado.version_info >= MIN_TORNADO
+except (ImportError, AttributeError, AssertionError) as e:  # pragma: no cover
+    raise ImportError(
+        _("The Jupyter Server requires tornado >=%s.%s.%s", *MIN_TORNADO)


This still isn't handling things correctly. How about we just cut to the chase?

Suggested change

_("The Jupyter Server requires tornado >=%s.%s.%s", *MIN_TORNADO)

_(f"The Jupyter Server requires tornado >= {MIN_TORNADO[0]}.{MIN_TORNADO[1]}.{MIN_TORNADO[2]}")

bollwyvl · 2020-11-16T18:51:03Z

Sorry, haven't i18n'd in a while, am used to some other system, maybe django, where you can pass the variable parts directly. Reading docs and verifying locally, I see it should be... _("yadda yadda %s.%.s.%s") % min_tornado so that it doesn't have to be re-translated every time the number changes

kevin-bates

Thanks Nick!

Zsailer

Thanks, @bollwyvl! LGTM.

Pin 1.0.x to tornado 6.1, remove asyncio patch [backport pieces of #339]

-no-entrypoint Remove pytest11 entrypoint and plugin, require tornado 6.1, remove asyncio patch, CI work

remote pytest11 entrypoint

3617344

bollwyvl mentioned this pull request Nov 13, 2020

pytest_tornasync becomes hard dependency if pytest is installed #337

Closed

try dotted import

ec6a9c6

bollwyvl changed the title ~~remote pytest11 entrypoint~~ remove pytest11 entrypoint Nov 13, 2020

add conftest to example

f35144d

kevin-bates mentioned this pull request Nov 13, 2020

basic approach for separate pytest plugin package #338

Closed

kevin-bates reviewed Nov 13, 2020

View reviewed changes

bollwyvl closed this Nov 13, 2020

bollwyvl reopened this Nov 13, 2020

bollwyvl added 3 commits November 13, 2020 17:57

merge master

c076f84

remove un-needed conftest.py

d7709fa

tweak some metadata

2a3a2a0

bollwyvl commented Nov 13, 2020

View reviewed changes

update fixtures in example

b96c939

bollwyvl added 3 commits November 13, 2020 19:59

actually use renamed fixtures

933b4f5

add some more ci trappings

20ec9ec

recursive-include to grab spec.yaml

d925e67

bollwyvl added 10 commits November 13, 2020 20:59

inject server config properly

fd94f12

fix workflow

3c99dff

collect coverage

02579a7

paramaterize (un)hidden file tests

47f07a4

log some more

ba40f81

try adding winpty

d026e83

fix pywinpty name

b86e72d

rever winpty, try longer request timeouts

631e075

just connect_timeout

0d0ac3c

try with unlimited connect_timeout

8694e53

bollwyvl added 2 commits November 14, 2020 12:31

overload jp_fetch

20816c4

fix imports

0e8a00f

bollwyvl added 2 commits November 14, 2020 13:32

try new tornado hotness

480d6d6

more work on removing asyncio patch

a0640b3

bollwyvl changed the title ~~remove pytest11 entrypoint~~ Remove pytest11 entrypoint and plugin, require tornado 6.1, remove asyncio patch, CI work Nov 14, 2020

kevin-bates reviewed Nov 15, 2020

View reviewed changes

updates from review

da0fef5

kevin-bates reviewed Nov 15, 2020

View reviewed changes

fix minimum tornado error message

e09ba05

kevin-bates reviewed Nov 16, 2020

View reviewed changes

move min tornado outside translation block

2ce063a

kevin-bates approved these changes Nov 16, 2020

View reviewed changes

kevin-bates mentioned this pull request Nov 17, 2020

pytest fixture base_url conflict with pytest_base_url #322

Closed

danielfrg mentioned this pull request Nov 17, 2020

Fix CI for jupyter-server danielfrg/jupyter-flex#74

Merged

Zsailer approved these changes Nov 17, 2020

View reviewed changes

Zsailer merged commit 5c31111 into jupyter-server:master Nov 17, 2020

bollwyvl mentioned this pull request Dec 8, 2020

Exclude tests from dist jupyter/nbclassic#36

Merged

Zsailer mentioned this pull request Dec 8, 2020

Pin 1.0.x to tornado 6.1, remove asyncio patch [backport pieces of #339] #355

Merged

Zsailer added a commit that referenced this pull request Dec 8, 2020

Merge pull request #355 from Zsailer/1.0.x

b4e0474

Pin 1.0.x to tornado 6.1, remove asyncio patch [backport pieces of #339]

This was referenced Dec 11, 2020

do not apply asyncio patch for tornado >=6.1 jupyter/notebook#5907

Merged

don't patch event loop for tornado 6.1+ jupyterlab/jupyterlab_server#142

Merged

Zsailer added a commit to Zsailer/jupyter_server that referenced this pull request Nov 18, 2022

short kernel heartbeat interval (jupyter-server#339)

b005dfd

hMED22 pushed a commit to hMED22/jupyter_server that referenced this pull request Jan 23, 2023

Merge pull request jupyter-server#339 from bollwyvl/jupyter-servergh-337

8d22a05

-no-entrypoint Remove pytest11 entrypoint and plugin, require tornado 6.1, remove asyncio patch, CI work

		@@ -1 +1 @@
		pytest_plugins = ['pytest_jupyter_server'] No newline at end of file
		pytest_plugins = ['jupyter_server.pytest_plugin']

	_("The Jupyter Server requires tornado >=%s.%s.%s", *MIN_TORNADO)
	_(f"The Jupyter Server requires tornado >= {MIN_TORNADO[0]}.{MIN_TORNADO[1]}.{MIN_TORNADO[2]}")

Conversation

bollwyvl commented Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevin-bates commented Nov 13, 2020

Uh oh!

bollwyvl commented Nov 14, 2020

Uh oh!

bollwyvl commented Nov 14, 2020

Uh oh!

bollwyvl commented Nov 14, 2020

Uh oh!

kevin-bates left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bollwyvl commented Nov 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bollwyvl commented Nov 16, 2020 via email

Uh oh!

kevin-bates left a comment

Choose a reason for hiding this comment

Uh oh!

Zsailer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bollwyvl commented Nov 13, 2020 •

edited

Loading