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

Run pytest for src tree if no filename is given #35999

Merged
merged 6 commits into from
Aug 5, 2023

Conversation

kwankyu
Copy link
Collaborator

@kwankyu kwankyu commented Jul 29, 2023

This fixes the failure seen, for example,

https://github.com/sagemath/sage/actions/runs/5678761729/job/15389749593?pr=35991

Note that the last failure message

local/var/lib/sage/venv-python3.8/lib/python3.8/site-packages/matplotlib/tests/__init__.py:6: in <module>
    raise IOError(
E   OSError: The baseline image directory does not exist. This is most likely because the test data is not installed. You may need to install matplotlib from source to get the test data.
=========================== short test summary info ============================
ERROR  - OSError: The baseline image directory does not exist. This is most likely because the test data is not installed. You may need to install matplotlib from source to get the test data.
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
============================== 1 error in 12.78s ===============================
Error: Process completed with exit code 2.

This is not a fault of matplotlib! The fault lies on our pytest, which blindly runs all tests found in the directory tree under the root dir. (matplotlib test files are supposed to run in a development install, where the baseline image directory exists, but our install (SPKG) of matplotlib is not a development install, and hence there's no baseline image directory.)

So we need to stop pytest from running blind. The failure in the incremental test workflow results from an error in sage-runtests. The relevant lines are

        exit_code_pytest = 0
        import pytest
        pytest_options = []
        if args.verbose:
            pytest_options.append("-v")
        # #31924: Do not run pytest on individual Python files unless
        # they match the pytest file pattern.  However, pass names
        # of directories. We use 'not os.path.isfile(f)' for this so that
        # we do not silently hide typos.
        filenames = [f for f in args.filenames
                     if f.endswith("_test.py") or not os.path.isfile(f)]
        if filenames or not args.filenames:   # <------------------------------------ faulty line!
            print(f"Running pytest on {filenames} with options {pytest_options}")
            exit_code_pytest = pytest.main(filenames + pytest_options)
            if exit_code_pytest == 5:
                # Exit code 5 means there were no test files, pass in this case
                exit_code_pytest = 0  

Look at the faulty line. The condition test allows pytest run without explicit filenames. This results in running pytest for all test files found under the (sage) root dir. Hence pytest attempts to run tests in matplotlib in local/var/lib/sage/venv-python3.11/lib/python3.11/site-packages/matplotlib/tests.

The branch of this PR fixes the problem.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@kwankyu kwankyu requested a review from mkoeppe July 29, 2023 00:39
@kwankyu
Copy link
Collaborator Author

kwankyu commented Jul 29, 2023

I also think that this part

      - name: Pytest
        if: contains(github.ref, 'pytest')
        run: |
          ../sage -python -m pip install coverage pytest-xdist
          ../sage -python -m coverage run -m pytest -c tox.ini --doctest-modules || true
        working-directory: ./worktree-image/src
        env:
          # Increase the length of the lines in the "short summary"
          COLUMNS: 120

has the same problem. I think the working directory should be working-directory: ./worktree-image/src/sage

src/bin/sage-runtests Outdated Show resolved Hide resolved
@tobiasdiez
Copy link
Contributor

Adding addopts = --ignore=local (or ../local) in https://github.com/sagemath/sage/blob/develop/src/tox.ini#L258 should also work, and is perhaps easier.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jul 29, 2023

Adding addopts = --ignore=local (or ../local) in https://github.com/sagemath/sage/blob/develop/src/tox.ini#L258 should also work, and is perhaps easier.

Explicitly limiting tests to src tree seems safe, as done in the Pytest section of the workflow.

@kwankyu kwankyu changed the title Do not run pytest without filenames Run pytest for src tree when no filename argument Jul 30, 2023
@kwankyu kwankyu changed the title Run pytest for src tree when no filename argument Run pytest for src tree for no filename argument Jul 30, 2023
@kwankyu kwankyu changed the title Run pytest for src tree for no filename argument Run pytest for src tree if no filename is given Jul 31, 2023
src/bin/sage-runtests Outdated Show resolved Hide resolved
Copy link
Contributor

@mkoeppe mkoeppe left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.
Tobias's suggestion to have pytest ignore local (and I think also venv) is a good idea too, but it's already a good fix as is.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Aug 2, 2023

Thanks!

@github-actions
Copy link

github-actions bot commented Aug 2, 2023

Documentation preview for this PR (built with commit e17f1de; changes) is ready! 🎉

@vbraun vbraun merged commit 6617b49 into sagemath:develop Aug 5, 2023
11 of 13 checks passed
@mkoeppe mkoeppe added this to the sage-10.1 milestone Aug 5, 2023
@kwankyu kwankyu deleted the p/fix-pytest-error branch August 24, 2023 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants