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

tests: constrain http pytest to tests/http directory #14611

Closed
wants to merge 1 commit into from

Conversation

jan2000
Copy link
Contributor

@jan2000 jan2000 commented Aug 20, 2024

Running the http pytest had to be done from tests directory or above, because the repeat argument fixture was defined in tests/conftest.py. However, the repeat argument is not needed because its functionality can be provided by pytest-repeat as documented in the test's README.md. So, removed the pytest_addoption function for the repeat argument and the pytest_report_header function is moved to tests/http/conftest.py.

TODO: Remove repeat argument from all tests. As a stopgap, a one-element list is defined for it for now.

@github-actions github-actions bot added tests CI Continuous Integration labels Aug 20, 2024
@bagder bagder requested a review from icing August 21, 2024 05:51
@icing
Copy link
Contributor

icing commented Aug 21, 2024

This change makes the reported header no longer appear unless pytest is started from tests/http. What are the advantages of this change?

Also, since --repeat=n no longer works, could you explain how you achieve the same with your change?

Running the http pytest had to be done from tests directory or above,
because the repeat argument fixture was defined in tests/conftest.py.
However, the repeat argument is not needed because its functionality
can be provided by pytest-repeat as documented in the test's
README.md. So, removed the pytest_addoption function for the repeat
argument and the pytest_report_header function is moved to
tests/http/conftest.py.

TODO: Remove repeat argument from all tests. As a stopgap, a
one-element list is defined for it for now.
@jan2000
Copy link
Contributor Author

jan2000 commented Aug 21, 2024

This change makes the reported header no longer appear unless pytest is started from tests/http. What are the advantages of this change?

The whole of the http testsuite lives within the tests/http directory. So it would be logical that tests/http is the "root" for it, and that the testsuite is fully contained within and can be run from that directory. Except there is tests/conftest.py that is outside of that "root", which is then not taken into account and thus can cause problems (see below).

Now the proper "root" directory of something can be considered a matter of opinion. I prefer a clear separation of the http pytest testsuites from the other C testsuites, and thus prefer properly containing it in a sub-directory.

Note however, that this is not the main thing about this PR. Though I titled this PR: constrain http pytest to tests/http directory, the main thing of this PR is: though documented that the testsuites can be properly run from within tests/http, it currently can not.

Also, since --repeat=n no longer works, could you explain how you achieve the same with your change?

From tests/http/README.md:

Several test cases can be repeated, they all have the repeat parameter (install pytest-repeat module). To make this work, you have to start pytest in the test directory itself (for some unknown reason). Like in:

curl/tests/http> pytest -k "test_02_06 and h2" --count=100

So repeating can be done with the --count parameter (and the then very useful --maxfail=1). However, the repeat argument (fixture) on the tests is not needed for that. In fact, the repeat argument on the tests is what breaks them when run from tests/http (ERROR: fixture 'repeat' not found), because the repeat fixture is defined in tests/conftest.py and is thus then not taken into account.

The proper fix is to remove the repeat argument everywhere. Before that is done, as a stopgap the repeat fixture is defined in tests/http/conftest.py as a dummy, a one-element list.

@icing
Copy link
Contributor

icing commented Aug 22, 2024

Thanks for the explanation. I think I can live with the report not showing up from the curl directory.

@bagder bagder closed this in a415286 Aug 22, 2024
@jan2000 jan2000 deleted the pytest-dir branch August 22, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration tests
Development

Successfully merging this pull request may close these issues.

2 participants