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

[WIP] Add Terminado tests #21

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

GeorgianaElena
Copy link
Collaborator

@GeorgianaElena GeorgianaElena commented Oct 29, 2020

This PR:

  • Sets up the testing infra with GHA (hope that's ok)
  • Should test the terminado client

Some things to consider:

  • The URL class from yarl seems to be escaping the urls if the flag encoded=True is not provided.
    Because the NotebookSSHServer doesn't use this flag, when trying to connect to http://localhost:12341/a%40b/, where the notebook runs, the url got escaped and got a 404.
  • The terminado tests use the NotebookTestBase class

I wanted to open the PR early because I'm a bit stuck with testing writing to stdin (as suggested in #13), I don't seem to find the file I'm trying to create anywhere 😕

Fixes #13
Fixes 2i2c-org/upstream#27

Copy link
Collaborator

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Nice work on this @GeorgianaElena! I'm still wrapping my head around SSH, Terminado, etc but I understand that this test suite is slimmed enough to not include the complexities of JupyterHub - I think thats great!

I'd be happy to assist with review efforts etc to get this landed. I'm interested in trying to resolve #41, and having some additional tests to verify I'm not breaking things while doing so would be great!

Comment on lines +14 to +21
@pytest.fixture
async def notebook():
# Start the notebook
notebook = NotebookTestBase()
notebook.setup_class()
notebook.wait_until_alive()
yield notebook
notebook.teardown_class()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that jupyter_server seem to have an equivalent. I'm not sure if its relevant to transition to that or not though.

Related issue: elyra-ai/elyra#831 (comment)

Related fixture from jupyter_server: https://github.com/jupyter-server/jupyter_server/blob/7d2f04f30e06e6efed90072df19824d416cb7129/jupyter_server/pytest_plugin.py#L185-L210

@GeorgianaElena
Copy link
Collaborator Author

Thanks @consideRatio! I will refresh the info in this PR in my head and come back with more info on why/where I got blocked with it later today. Hopefully we can figure it out together!

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.

Add testing infrastructure for JupyterHub SSH Write terminado tests
2 participants