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

ENH: Configure pytest to use DTChecker #71

Merged
merged 14 commits into from
Jul 24, 2023

Conversation

Sheila-nk
Copy link
Collaborator

@Sheila-nk Sheila-nk commented Jul 19, 2023

  • implemented the pytest_configure hook to register DTChecker as a plugin thus allowing pytest to use it during doctest execution.
  • overrode the _get_checker function to return the DTChecker.
  • added test cases to assert that pytest utilizes the DTChecker which handles numpy's intricacies.
  • changed the default_namespace attribute to {} for explicit import of numpy in test examples.
  • explicitly imported numpy in all docstring test examples.
  • added a CI test that performs doctests through the plugin.

Closes #72

@Sheila-nk
Copy link
Collaborator Author

@ev-br
I might require some assistance here.
The test to ensure that doctest uses the DTChecker is failing and I am uncertain why.
It seems the DTChecker is not being utilized to handle array abbreviation in this case.

Screenshot 2023-07-20 at 13 22 26 Screenshot 2023-07-20 at 13 22 59

Copy link
Member

@melissawm melissawm left a comment

Choose a reason for hiding this comment

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

I don't see the failure locally, so not sure how to reproduce 🤔

Sheila-nk and others added 2 commits July 20, 2023 17:52
Co-authored-by: Melissa Weber Mendonça <melissawm@gmail.com>
@Sheila-nk Sheila-nk marked this pull request as ready for review July 21, 2023 08:21
@Sheila-nk Sheila-nk changed the title WIP: Configure pytest to use DTChecker ENH: Configure pytest to use DTChecker Jul 21, 2023
Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

I really like the fact that it now reuses existing test cases!

I also like the fact that the patch to switch on DTChecker is a three-liner.

That said:

  • can you add a test using pytest CLI on CI? Both to make sure this works with $ pytest ... CLI interface, and to serve as a canonical reference of how to use it. Have a look at at https://github.com/ev-br/scpdt/blob/main/.github/workflows/pip.yml, it has several steps for various ways to run it the "classic" way.
  • Currently it seems to monkey-patch the pytest plugin, correct? Would be good to understand how robust this is for future maintenance. Are we reaching into private pytest details which can change in a future pytest version?
  • Would be good to look at prior art: pytest-doctestplus and xdoctest. Are they doing similar things? If they do things differently, then how? Are their ways a historic baggage or more robust? Etc.

@Sheila-nk
Copy link
Collaborator Author

Sheila-nk commented Jul 21, 2023

can you add a test using pytest CLI on CI? Both to make sure this works with $ pytest ... CLI interface, and to serve as a canonical reference of how to use it.

  • added python -m pytest for now to run tests intest_pytest_configuration. Normally it would be python -m pytest --doctest-modules to run doctests but to test the plugin, we have already passed the cli argument to the inline_run function. Is it a better idea to run tests in the test_pytest_configuration module only?
    python -m pytest scpdt/tests/test_pytest_configuration.py

Currently it seems to monkey-patch the pytest plugin, correct? Would be good to understand how robust this is for future maintenance. Are we reaching into private pytest details which can change in a future pytest version?

Would be good to look at prior art: pytest-doctestplus and xdoctest. Are they doing similar things? If they do things differently, then how? Are their ways a historic baggage or more robust?

  • so from what I have seen, it seems both plugins monkey patch the pytest plugin but in different ways. xdoctest completely disables the doctest plugin and prevents it from collecting tests. pytest-doctestplus does the same thing I did, just plugging in the doctestplus tools where necessary by overriding doctest's functions and classes.

@ev-br
Copy link
Member

ev-br commented Jul 24, 2023

Thanks for the explanations Sheila.

Would be nice to somehow check / make sure that the added $ python -m pytest CI line goes through the plugin. How is this different from the existing https://github.com/ev-br/scpdt/blob/main/.github/workflows/pip.yml#L48 ?

so from what I have seen, it seems both plugins monkey patch the pytest plugin but in different ways. xdoctest completely disables the doctest plugin and prevents it from collecting tests. pytest-doctestplus does the same thing I did, just plugging in the doctestplus tools where necessary by overriding doctest's functions and classes.

Makes sense!
xdoctests's main selling point is indeed an AST-based collection, so it makes sense it disables the plugin.
That pytest-doctestplus also does monkey-patching is a good point, will be nice to document implementation choices and compare them to other plugins. Maybe done separately later when a general picture emerges.

All that said, this pull request LGTM. Feel free to merge if it helps iterating (or keep adding things to this branch, whichever works for you @Sheila-nk).

@Sheila-nk
Copy link
Collaborator Author

Thanks for your review @ev-br 😄
I think I would like to make one more change in this PR for the CI test based on this:

Would be nice to somehow check / make sure that the added $ python -m pytest CI line goes through the plugin. How is this different from the existing https://github.com/ev-br/scpdt/blob/main/.github/workflows/pip.yml#L48 ?

Thanks for catching this. So to ensure that python -m pytest goes through the plugin, we add the pytest cli argument --doctest-modules. Normally, the existing test cases would fail since pytest would use doctest's Output_Checker. But since we monkey-patched pytest to use DTChecker instead, the test cases will pass. To ensure the same is reflected on the CI, I will need to exclude the plugin's test module: test_pytest_configuration.

@Sheila-nk
Copy link
Collaborator Author

For now, I have added the CI test that performs doctesting on the module_cases test cases only since that is one that solely relies on the Checker used.
Let me know if that is okay. I will update the CI tests as we continue plugging in more scpdt tools.

Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

For now, I have added the CI test that performs doctesting on the module_cases

Confirmed locally that

$ python -m pytest scpdt/tests/module_cases.py --doctest-modules

passes in a venv where scpdt is installed and fails in a venv where it is not. Great!

Optionally, might want to add a more explicit comment, along the lines of "fails unless the plugin is activated".

@Sheila-nk
Copy link
Collaborator Author

Oh! sorry @ev-br . Instead of quoting your comment, I accidentally edited it, but I have restored it. Apologies!!

Optionally, might want to add a more explicit comment, along the lines of "fails unless the plugin is activated".

Yes. Maybe it would also be a good idea to start updating the documentation.
Have a section for the pytest plugin?

@ev-br
Copy link
Member

ev-br commented Jul 24, 2023

Yes for updating the documentation. This PR or a sequel, up to you.

@Sheila-nk
Copy link
Collaborator Author

Great! Will update the documentation in a different PR.

@Sheila-nk Sheila-nk added the enhancement New features w.r.t. the original refguide-check label Jul 24, 2023
@Sheila-nk
Copy link
Collaborator Author

Thanks again @ev-br for the review and approval.
Will now merge this PR 🚀

@Sheila-nk Sheila-nk merged commit f054b30 into scipy:main Jul 24, 2023
@Sheila-nk Sheila-nk deleted the plugin-checker branch July 24, 2023 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features w.r.t. the original refguide-check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure pytest to use the DTChecker
3 participants