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

pytest: DX - Support targeting specific tests using make pytest w/ PYTEST_TESTS #7681

Conversation

s373nZ
Copy link
Contributor

@s373nZ s373nZ commented Sep 19, 2024

This is a small PR intended to slightly improve developer experience. When running the Pytest integration testing suite locally using the command make pytest, the build system takes care of altering the local context variable PYTHONPATH to make sure contributed modules are accessible to the process. While this works great for running the entire test suite, oftentimes developers are concerned with repeatedly running a subset of the tests, the particular test relevant to the functionality they are working on, or some problematic tests which the suite reports failing.

Currently, in order to target specific tests for a pytest run, a developer might:

  1. Run make pytest
  2. Observe the output to discern and capture the computed value for PYTHONPATH
  3. Copy this value
  4. Export the value as a local PYTHONPATH variable
  5. Run pytest outside of the Makefile context and feeding in the targeted list of files and tests cases to the local command

This PR suggests supporting the pass-through of a variable named PYTEST_TESTS which defaults to tests/ and can be set in a similar fashion to PYTEST_PAR and PYTEST_OPTS. This would allow developers to skip a few steps in setting up their local environment for running pytest and enjoy some of the Makefile's sane defaults if desired. For example,

PYTEST_TESTS="tests/test_askrene.py::test_layers" make pytest

Certainly, experienced CLN developers already have more convenient workflows, so please let me know if there is a more accessible convention for this.

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

  • The changelog has been updated in the relevant commit(s) according to the guidelines.
  • Tests have been added or modified to reflect the changes.
  • Documentation has been reviewed and updated as needed.
  • Related issues have been listed and linked, including any that this PR closes.

@s373nZ s373nZ force-pushed the make-pytest-support-target-tests branch from 356ec4c to 340385c Compare September 23, 2024 17:10
@ShahanaFarooqui
Copy link
Collaborator

@s373nZ This functionality is already achievable by:

  • Running a particular test: pytest tests/test_askrene.py::test_layers
  • Running all tests in a specific file: pytest tests/test_askrene.py

A typical command for me to run a test looks like:

rm -rf /tmp/ltests* && make -s && VALGRIND=0 TIMEOUT=10 TEST_DEBUG=1 pytest -n 8 -vv tests/test_runes.py::test_createrune
  • The first command clears old test files.
  • The second compiles the code.
  • Finally, the pytest command runs tests with various options.

Some options available in the code are:

  • TIMEOUT
  • PYTEST_PAR
  • TEST_DEBUG
  • VALGRIND
  • TEST_NETWORK
  • TEST_DB_PROVIDER
  • TEST_CHECK_DBSTMTS
  • COMPAT
  • SLOW_MACHINE
  • DEPRECATED_APIS
  • EXPERIMENTAL_DUAL_FUND
  • EXPERIMENTAL_SPLICING

While the testing documentation could be expanded with additional details, should we close this PR, or is there something I am overlooking?

@s373nZ
Copy link
Contributor Author

s373nZ commented Sep 25, 2024

@ShahanaFarooqui Thank you very much for the example and comprehensive explanation. That definitely helps to understand more how you are using it more in practice.

As I mentioned in the PR description, under my setup, I have to manually fix the PYTHONPATH variable in order to run the tests using the pytest command. Possibly, that is because I experiment with a variety of local version setups, including asdf and nix. It's likely these contexts affect my PYTHONPATH in an unexpected way.

Personally, I still think there's a case to be made for this PR in the spirit of "it doesn't hurt", but I'm totally fine with closing this PR if you like. It was just a small nuisance and improvement I thought to offer. Thanks for the consideration.

@ShahanaFarooqui
Copy link
Collaborator

@s373nZ I missed the part about needing to export PYTHONPATH locally, possibly due to my unfamiliarity with nix and asdf. Given that, it makes sense to include this variable. Thanks for clarifying!

I tested the change with & without PYTEST_TESTS and it works perfectly.

make VALGRIND=0 TIMEOUT=30 pytest
make VALGRIND=0 TIMEOUT=30 PYTEST_TESTS="tests/test_runes.py" pytest
make VALGRIND=0 TIMEOUT=30 PYTEST_TESTS="tests/test_runes.py::test_createrune" pytest

We can implement this change, but please ensure to include the relevant details in the testing documentation as well.

@ShahanaFarooqui ShahanaFarooqui added this to the v24.11 milestone Sep 25, 2024
@s373nZ
Copy link
Contributor Author

s373nZ commented Oct 1, 2024

@ShahanaFarooqui I added the usage of PYTEST_TESTS to the testing documentation. I also enumerated out the additional environment variable options you shared in #7681 (comment) while I was there and attempted to describe them with my best understanding of their purpose. Let me know if any of them need changes as well.

@s373nZ s373nZ force-pushed the make-pytest-support-target-tests branch from 8df46dc to 951d07e Compare October 1, 2024 15:00
@s373nZ s373nZ force-pushed the make-pytest-support-target-tests branch 2 times, most recently from 94cbd2e to 2e0c523 Compare October 2, 2024 09:53
@s373nZ
Copy link
Contributor Author

s373nZ commented Oct 2, 2024

Moved the line re: PYTEST_TESTS down a bit in the documentation to fit context appropriately.

@ShahanaFarooqui
Copy link
Collaborator

ShahanaFarooqui commented Oct 2, 2024

Merged two doc: Include documentation for test targeting via PYTEST_TESTS. commits in one.

ACK 5d4234f.

Select tests by setting the `PYTEST_TESTS` environment variable.

Changelog-None
Also update the table of optional environment variables to set when
running the integration tests.
@ShahanaFarooqui ShahanaFarooqui merged commit fea3049 into ElementsProject:master Oct 4, 2024
38 checks passed
@s373nZ s373nZ deleted the make-pytest-support-target-tests branch October 14, 2024 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants