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

pytest fixture base_url conflict with pytest_base_url #322

Closed
danielfrg opened this issue Oct 25, 2020 · 15 comments · Fixed by #335
Closed

pytest fixture base_url conflict with pytest_base_url #322

danielfrg opened this issue Oct 25, 2020 · 15 comments · Fixed by #335

Comments

@danielfrg
Copy link

Looks like this package now defines a pytest fixture named base_url that conflicts with another plugin https://pypi.org/project/pytest-base-url/ that defines the same fixture but with different scope

I started to get this error from a CI job here with the following logs:

ScopeMismatch: You tried to access the 'function' scoped fixture 'base_url' with a 'session' scoped request object, involved factories
../../../../__t/Python/3.8.6/x64/lib/python3.8/site-packages/pytest_base_url/plugin.py:19:  def _verify_url(request, base_url)
../../../../__t/Python/3.8.6/x64/lib/python3.8/site-packages/jupyter_server/pytest_plugin.py:205:  def base_url()

Not sure if there is a better way to fix this other than renaming the fixture here to something like jupyter_base_url.

@kevin-bates
Copy link
Member

This certainly does seem like an issue and a rename seems appropriate. I'm wondering if js_base_url might be better in that jupyter_base_url has the potential to conflict with other projects (should we all go crazy with pytest-plugins everywhere 😄 ).

Did you want to make these changes?

I suppose one issue is that jupyter_server-extension-based projects might be referencing base_url today in their tests, but I'm not sure there's anything we can do about that.

@echarles and @Zsailer have spent time in this area, do you see another way to address this?

@goanpeca
Copy link
Contributor

Can we check for available fixtures and provide a message in case it is found, so at least if we break things we provide a path moving forward.

Is base_url meant to be used by other projects or is it an internal detail? We could _base_url also?

@danielfrg
Copy link
Author

Is base_url meant to be used by other projects or is it an internal detail? We could _base_url also?

Is is meant to be used by external systems, that lib also adds a flag to control the value of the base_url. I like _base_url personally.

If we decide on that I can make a PR to change this.

@kevin-bates
Copy link
Member

There is a use of base_url in lab. Also note on that same line is jupyter server's auth_header fixture. Likewise, I know project Elyra's metadata handler tests use server's fetch, environ, and server_config fixtures.

Since server purposely exposes its pytest_plugins.py file for use by extension authors (i.e., is a "fixture provider"), we may want to entertain prefixing a number of fixtures (all perhaps?) thereby protecting our consumers from potential collisions they may encounter. Thepytest-xxx packages essentially have free reign over the non-prefixed namespace. Since there's precedent for a jpserver_ prefix to extension authors, I think we should go with that.

@danielfrg - is your project blocked by this? I'm wondering if we shouldn't just open a PR, prefix base_url (and obvious others) and discuss how to proceed further in Thursday's server meeting. Since Lab is close to releasing their 3.0 release (which uses jupyter server), we should make sure this is coordinated.

@danielfrg
Copy link
Author

All that makes sense.

Its relatively blocked because tests are not going to pass unless i either remove the pytest-plugin or we fix it on jupyter_server.

@kevin-bates
Copy link
Member

In talking with a colleague about this, they suggested that we formalize server's pytest-plugins and create a package: pytest-jupyter-server (or something similar) and use a shorter prefix of jp_ (which is the precedent used for managing CSS namespaces). At any rate, we decided to make tomorrow's team meeting a "working meeting" to address this issue. I apologize for the run-around but it would be good to address this correctly (and completely) the first time.

@danielfrg
Copy link
Author

Thanks for the comments and for getting together to fix this!
Let me know if I can help somehow.

@kevin-bates
Copy link
Member

Right on - thank you for your patience on this. Please feel free to join tomorrow's meeting if you're available.

@Zsailer
Copy link
Member

Zsailer commented Nov 2, 2020

We discussed this our out last Jupyter Server meeting, @danielfrg.

We're working on a separate pytest-plugin library, pytest-jupyter, that other projects (like yours) can use for testing. When I originally created the jupyter server plugin, I didn't expect that it would be used outside this library. We're learning now that it's quite useful for extensions (e.g. jupyterlab, nbclassic, etc). This new plugin library will properly namespace fixtures (using a prefix) so there is little chance of fixture collisions. We should have this plugin released by the end of the week (hopefully sooner).

Thank you for the patience on this!

@Zsailer
Copy link
Member

Zsailer commented Nov 3, 2020

@danielfrg—OK, you should be able to try out pytest-jupyter now. It provides all the fixtures you need to test your extension with jupyter_server. You'll need to update the fixture names with the prefix jp_, e.g. jp_fetch.

We'll likely release this package on PyPI later this week (and add documentation). For now, you can point at pytest-jupyter's main branch to get your CI to pass.

@danielfrg
Copy link
Author

Thanks for the release!

I still see that base_url (and other fixtures) are still in this codebase: https://github.com/jupyter/jupyter_server/blob/master/jupyter_server/pytest_plugin.py#L205-L207

This would probably make it so they are still incompatible with other pytest plugins, is this correct?

@kevin-bates
Copy link
Member

kevin-bates commented Nov 17, 2020

That particular fixture is renamed to jp_base_url via the pytest-jupyter.pytest-jupyterserver plugin. But, yeah, since jupyter_server currently publishes a pytest plugin, the conflict will still occur, unfortunately. This will be addressed by #339. In the meantime, if you build that branch, you should be able to restore your use of base_url.

@danielfrg
Copy link
Author

Makes sense, thank you! I will wait for that branch to be merged.

@Zsailer
Copy link
Member

Zsailer commented Dec 16, 2020

@danielfrg, after a few iterations on our pytest plugin (and dependencies), Jupyter Server 1.1 was released with a new pytest plugin. All fixtures have been prefixed with jp_. The entrypoint for the plugin was removed so that pytest dependencies aren't required for package installation.

To use this plugin for your extension, install the test dependencies of jupyter_server:

pip install jupyter_server[test]

then add the plugin to your conftest.py pytest_plugin variable:

# conftest.py
pytest_plugin = [
    "jupyter_server.pytest_plugin",
    ...
]

I'll add this info to our documentation sometime today.

@danielfrg
Copy link
Author

Thanks for the heads up it totally fixed my issue.
Thank you all!

Zsailer added a commit to Zsailer/jupyter_server that referenced this issue Nov 18, 2022
* bump pycryptodome

* encode strings for shared keys
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 a pull request may close this issue.

4 participants