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

Assert return value of expected_http_error #1308

Merged
merged 2 commits into from Aug 15, 2023

Conversation

toslunar
Copy link
Contributor

expected_http_error returns bool | None to be tested.

def expected_http_error(error, expected_code, expected_message=None):
"""Check that the error matches the expected output error."""
e = error.value
if isinstance(e, HTTPError):
if expected_code != e.status_code:
return False
if expected_message is not None and expected_message != str(e):
return False
return True
elif any(
[
isinstance(e, HTTPClientError),
isinstance(e, HTTPError),
]
):
if expected_code != e.code:
return False
if expected_message:
message = json.loads(e.response.body.decode())["message"]
if expected_message != message:
return False
return True

@welcome
Copy link

welcome bot commented Jul 31, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

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! 👋

Welcome to the Jupyter community! 🎉

@Zsailer
Copy link
Member

Zsailer commented Aug 10, 2023

Thanks you @toslunar!

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

@matthewwiese
Copy link
Collaborator

@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:

  • payload_3, payload_4, and payload_5 are all missing one of data, version, or schema_id
  • payload_6 has a timestamp entry not formatted in ISO with UTC offset (see here)

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
Copy link
Member

Zsailer commented Aug 10, 2023

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
Copy link
Collaborator

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

@toslunar
Copy link
Contributor Author

@matthewwiese Thank you for fixing the code!

@matthewwiese
Copy link
Collaborator

@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
Copy link
Collaborator

@matthewwiese I believe the errors are related to jupyterlab/jupyterlab_server#400.

Copy link
Collaborator

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thank you both!

@blink1073 blink1073 merged commit ed65a6c into jupyter-server:main Aug 15, 2023
33 of 35 checks passed
@welcome
Copy link

welcome bot commented Aug 15, 2023

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants