Skip to content

Conversation

@befeleme
Copy link
Contributor

Hey,
I propose some changes to the tests:

  1. Unskip tests using obsolete pytest yield feature. That is changed to recommended pytest.mark.parametrize.
  2. Remove DeprecationWarning caused by \d in regular expression - use recommended raw string instead.
  3. Remove PytestCollectionWarning failing on recognizing TestingStopApp - problematic part is ^Test* in the class name - renaming makes it work again.

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 @befeleme - these changes look great. I had the comment about the stop app class name so that the test subject (StopApp) is preserved.

passwd_check(nb.password, password)

class TestingStopApp(notebookapp.NbserverStopApp):
class StopTestApp(notebookapp.NbserverStopApp):
Copy link
Member

Choose a reason for hiding this comment

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

Since the purpose of this is to test the "StopApp", I would suggest a rename to StopAppTest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, fixing it.

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.

Looks good. Thank you.

@kevin-bates kevin-bates merged commit c719e27 into jupyter:master Mar 24, 2021
@befeleme befeleme deleted the fix-tests branch March 24, 2021 07:04
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants