ElementsProject / lightning

Core Lightning — Lightning Network implementation focusing on spec compliance and performance
Other
2.86k stars 905 forks source link

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

Closed s373nZ closed 1 month ago

s373nZ commented 1 month ago

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:

ShahanaFarooqui commented 1 month ago

@s373nZ This functionality is already achievable by:

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

Some options available in the code are:

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

s373nZ commented 1 month ago

@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 commented 1 month ago

@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.

s373nZ commented 1 month ago

@ShahanaFarooqui I added the usage of PYTEST_TESTS to the testing documentation. I also enumerated out the additional environment variable options you shared in https://github.com/ElementsProject/lightning/pull/7681#issuecomment-2374712463 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 commented 1 month ago

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

ShahanaFarooqui commented 1 month ago

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

ACK 5d4234fd4f77ae28448760d9a262e67624dff814.