Skip to content

Conversation

rao23
Copy link
Contributor

@rao23 rao23 commented Feb 25, 2025

This is a fix on the issue #139

…y - isinstance(type, dict) - 'type' is a built-in class for type which will never be an instance of dict type
@Zsailer
Copy link
Member

Zsailer commented Mar 13, 2025

I'll investigate test failures today/tomorrow (which are unrelated) and merge this as soon as that work is done. Thank you for the PR!

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

Good catch - thank you.

@ojarjur ojarjur added the bug Something isn't working label Jun 2, 2025
@rao23
Copy link
Contributor Author

rao23 commented Jun 17, 2025

The unit tests seem to be failing on the pre-existing issue - #108. This change is very simplistic and does not affect any of the failing targets. Can we skip these failing tests and push this through. Following suite of a similar PR - #142.

@ojarjur
Copy link
Collaborator

ojarjur commented Jun 17, 2025

@rao23 thanks for reaching out and for the question.

The failures that were ignored for #142 have all been fixed now (in #143), so the failing tests on this PR (after merging in the latest changes from main) should be unrelated to that.

To see whether or not the failures in this PR are caused by this change, I checked out both the current main branch and the merge commit for this PR and ran hatch run test:test in both setups.

For main, I get 3 tests passed and one expected failure (xfailed). However, for the PR commit I get 3 passed and 1 failed.

So, the change in this PR is the cause of that failure.

I don't know why (yet), but since the change was from "expected failure" to just "failure", I suspect that there is some sort of update to the test_lifecycle method here that we need to include as part of this PR.

@ojarjur
Copy link
Collaborator

ojarjur commented Jun 17, 2025

@rao23 thanks for reaching out and for the question.

The failures that were ignored for #142 have all been fixed now (in #143), so the failing tests on this PR (after merging in the latest changes from main) should be unrelated to that.

To see whether or not the failures in this PR are caused by this change, I checked out both the current main branch and the merge commit for this PR and ran hatch run test:test in both setups.

For main, I get 3 tests passed and one expected failure (xfailed). However, for the PR commit I get 3 passed and 1 failed.

So, the change in this PR is the cause of that failure.

I don't know why (yet), but since the change was from "expected failure" to just "failure", I suspect that there is some sort of update to the test_lifecycle method here that we need to include as part of this PR.

It looks like simply removing the xfail annotation from this line makes the hatch tests pass again.

I.E. this PR fixes the expected failure, so now we have to update the expectation.

Copy link
Member

@kevin-bates kevin-bates 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 @ojarjur!

@rao23
Copy link
Contributor Author

rao23 commented Jun 24, 2025

@kevin-bates I believe this PR is ready to be merged now, please let me know if anything else is needed from my end.

@kevin-bates kevin-bates merged commit bd87a2c into jupyter-server:main Jun 24, 2025
17 checks passed
@rao23 rao23 deleted the fix-kernelspecs-issue branch June 24, 2025 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants