jupyter-server / jupyter_server

The backend—i.e. core services, APIs, and REST endpoints—to Jupyter web applications.
https://jupyter-server.readthedocs.io
BSD 3-Clause "New" or "Revised" License
484 stars 295 forks source link

Assert return value of `expected_http_error` #1308

Closed toslunar closed 1 year ago

toslunar commented 1 year ago

expected_http_error returns bool | None to be tested. https://github.com/jupyter-server/jupyter_server/blob/c5dc0f696f376e1db5a9a0cbcebb40a0bf98875c/tests/utils.py#L24-L45

welcome[bot] commented 1 year ago

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly. welcome You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:

Zsailer commented 1 year ago

Thanks you @toslunar!

We looked at this in the Jupyter Server meeting this week and we'll follow up soon. cc @matthewwiese

matthewwiese commented 1 year ago

@Zsailer The four tests fail because of the use of try/except within post() (here)

test_post_event_400() is called using four payloads and is expected to fail because:

You can see in the error message for payload_6 that a 400 is correctly raised, but is caught by the try/except block and changed to a 500.

If the try/except block is modified to raise HTTPErrors as they are, then all tests pass:

try:
    validate_model(payload)
    self.event_logger.emit(
        schema_id=payload.get("schema_id"),
        data=payload.get("data"),
        timestamp_override=get_timestamp(payload),
    )
    self.set_status(204)
    self.finish()
except web.HTTPError:
    raise
except Exception as e:
    raise web.HTTPError(500, str(e)) from e
Zsailer commented 1 year ago

Thanks, @matthewwiese! The modification to the try/except block looks right to me.

I've added you to our list of collaborators—so you can push directly to this PR/branch. If you or @toslunar can update this PR to get the tests passing, I'm happy to merge. Thanks!

matthewwiese commented 1 year ago

Looks like the jupyterlab_server tests are failing (link) all with the same error: AttributeError: module 'jsonschema._utils' has no attribute 'load_schema'

toslunar commented 1 year ago

@matthewwiese Thank you for fixing the code!

matthewwiese commented 1 year ago

@toslunar Thank you for submitting a PR! :)

@Zsailer Do you have an idea for what's causing the jupyterlab_server tests to fail? I took a brief look and it appears it might be related to packages/dependencies?

blink1073 commented 1 year ago

@matthewwiese I believe the errors are related to https://github.com/jupyterlab/jupyterlab_server/issues/400.

welcome[bot] commented 1 year ago

Congrats on your first merged pull request in this project! :tada: congrats Thank you for contributing, we are very proud of you! :heart: