pyo3 icon indicating copy to clipboard operation
pyo3 copied to clipboard

pyo3-testing: initial proc_macro to make testing pyo3functions easier

Open MusicalNinjaDad opened this issue 1 year ago • 3 comments

Overview

Add #[pyo3test] to support easier testing of code wrapped by #[pyfunction]. See section 2.4 of the Guide: "Testing your code" for more details.

Concept

The idea is to make it easy for people to run final tests of their exported functions in rust (step 2 below). The test approach supported and documented in the Guide (new section 2.4) is:

  1. test your functionality in rust, by implementing it as pure rust functions and directly unit testing them.
  2. wrap those rust functions with #[pyo3...] and run simple unit tests in rust to validate the wrapping, that you got the type conversions and error handling correct etc.
  3. build your python project and run python tests with pytest or unittest on the final codebase. See also the original discussion in #3984

Implementation Details

This PR introduces a new crate pyo3-testing which is placed behind a feature gate testing. This is enabled by default and included in full.

The crate provides a single proc macro #[pyo3test] and also contains the implementation functionality and unit tests. As these are not made public, they can coexist in the same crate for simplicity (no additional "-backend" or "-procmacros" crate is needed).

At its simplest the macro takes a function (the "testcase") adds a #[test] attribute, initialises the interpreter and executes the function body wrapped by Python::with_gil(|py| {...}).

Additionally the user can request for one or more modules and/or functions to be imported and exposed as PyModule and PyFunctions. This is done by adding #[pyo3import(...)] attributes to the testcase.

Initialising and exposing the PyModules - unsafe codeblock + Python>=3.9

When running multiple tests on wrapped modules, interpreter initialisation becomes a problem. This issue will be experienced by anyone who is trying to test their wrapped functions in rust, whether they create the entire testcase themselves or use pyo3-testing:

  • If the tests are run with auto-initialize then the interpreter is guaranteed to be initialised and append_to_inittab!() will fail - in this case all tests are guaranteed to panic.
  • If the tests are run without auto-initialize then there is the risk that the interpreter will already be initialized by a different test - in this case the tests are simply flaky and will panic at random.

I experienced this issue continually with different fail points but no easy way to guarantee failure.

The solution to this involves a small unsafe block and is only possible on cPython >=3.9 - I will add a specific comment to that code with an explanation and to allow for a more focussed discussion / review.

Ideas for extending in future PRs

  • add py_raises! and call! macros to make it easier to write testcases. If these are macro-rules! macros then this will likely mean creating a pyo3_testing_procmacros crate, moving the current code there and re-exporting it from pyo3_testing in order to maintain the current API. For now working on the assumption of YAGNI and avoiding the additional complexity of a new crate, given that this should not require any breaking changes going forwards.
  • extend #[pyo3test] coverage to include classes.
  • add a cfg option and document a github action example to allow easy skipping of pyo3tests in CI for unsupported python versions.

ToDos

  • [X] ~~bugfix problems pre-initiatized interpreter~~
  • [x] ~~add dedicated comment to the module initialisation, resolve the current comment on that topic.~~
  • [X] ~~raise a separate PR for the additional github workflow, so it no longer shows up here as a change (where it doesn't really belong)~~
  • [X] ~~create UI tests for error handling proc_macros & potentially improve error handling based on the results~~
  • [X] ~~add a "testing your exported code" section to "2. Using Rust from Python" with How-To guidance~~
  • [X] ~~add an entry for this PR in newsfragments - see [https://pyo3.rs/main/contributing.html#documenting-changes]~~

Note

What would you like me to do about .github/workflows/ci-quick.yml? I've raised #4115 for that and a few updates to contributing.md. If you would like me to remove it from here I will do so when the review is complete as a final step (otherwise I run into issues with testing my code for any additional changes). If the other PR ik OK and merged before this then leaving it here is a no-op.

MusicalNinjaDad avatar Apr 19 '24 12:04 MusicalNinjaDad

Just confirming that I have seen this, but I'm generally quite behind on OSS reviews at the moment. Hoping to have a bit of a catch-up session tomorrow afternoon, please bear with🫣

davidhewitt avatar Apr 20 '24 07:04 davidhewitt

CodSpeed Performance Report

Merging #4099 will not alter performance

Comparing MusicalNinjaDad:pyo3-testing (48a29ad) with main (7263fa9)

Summary

✅ 68 untouched benchmarks

codspeed-hq[bot] avatar Apr 24 '24 14:04 codspeed-hq[bot]

Hi @davidhewitt

This is now ready for review in my opinion, there are a few additional ideas which I have documented at the end of the PR. I would be happy to continue working on this topic and support the pyo3-testing functionality if you are interested in including it.

I also understand that this is quite a large PR and you are busy. I hope it is worth your time when you do have a chance to look at it. If there is anything I can do to help make reviewing easier, please just let me know. Please also don't feel like you have to provide comments on everything at once, I'm happy to take any feedback in pieces if that suits you better.

MusicalNinjaDad avatar Apr 24 '24 14:04 MusicalNinjaDad