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

Revamp testing and coverage via tox #2440

Merged
merged 4 commits into from Mar 5, 2024

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented Mar 3, 2024

Changes

  • Reorients tox workflows for running pytest and evaluating coverage
  • The running of tests and reporting of coverage are now separate tox environments; additionally, running a py environment no longer automatically generates coverage
  • New tox environments/labels:
    • Pytest and coverage reporting: tox -m test or tox -m test310
    • Pytest and coverage reporting for all Pythons: tox
    • Run most of CI: tox -m ci
    • Only pytest: tox -e py
    • Only pytest for specific python: tox -e py310
    • Pytest with coverage tracking: tox -e py-cov or tox -e py310-cov
    • Coverage reporting: tox -e coverage

Notes

  • Technically, it is possible to use tox p to run the envs in parallel but this may causes errors due to conflicts during pip package installs and moving coverage database files

CC: @HalfWhitt

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@rmartin16 rmartin16 force-pushed the tox-pytest-cov branch 2 times, most recently from 90b7348 to b7a31cf Compare March 3, 2024 19:27
@rmartin16 rmartin16 force-pushed the tox-pytest-cov branch 2 times, most recently from 807dd08 to c8668bd Compare March 3, 2024 22:57
@rmartin16
Copy link
Member Author

rmartin16 commented Mar 3, 2024

Here's my changes to the testbed section of CI since it isn't rendering too well with the change to the number of tabs; notably, I updated the Android test to use the installed version of JDK 17:

   testbed:
-    runs-on: ${{ matrix.runs-on }}
+    name: Testbed
     needs: core
+    runs-on: ${{ matrix.runs-on }}
     strategy:
       fail-fast: false
       matrix:
         backend: [ "macOS-x86_64", "macOS-arm64", "windows", "linux", "android", "iOS" ]
         include:
-          - pre-command:
-            briefcase-run-prefix:
-            briefcase-run-args:
+          - pre-command: ""
+            briefcase-run-prefix: ""
+            briefcase-run-args: ""
             setup-python: true
 
           - backend: "macOS-x86_64"
             platform: "macOS"
             runs-on: "macos-12"
-            app-user-data-path: $HOME/Library/Application Support/org.beeware.toga.testbed
+            app-user-data-path: "$HOME/Library/Application Support/org.beeware.toga.testbed"
 
           - backend: "macOS-arm64"
             platform: "macOS"
             runs-on: "macos-14"
-            app-user-data-path: $HOME/Library/Application Support/org.beeware.toga.testbed
+            app-user-data-path: "$HOME/Library/Application Support/org.beeware.toga.testbed"
 
           # We use a fixed Ubuntu version rather than `-latest` because at some point,
           # `-latest` will be updated, but it will be a soft changeover, which would cause
@@ -195,7 +207,8 @@ jobs:
             # work either. Blackbox is the lightest WM we've found that works.
             pre-command: |
               sudo apt update -y
-              sudo apt install -y blackbox pkg-config python3-dev libgirepository1.0-dev libcairo2-dev gir1.2-webkit2-4.0
+              sudo apt install -y --no-install-recommends \
+                blackbox pkg-config python3-dev libgirepository1.0-dev libcairo2-dev gir1.2-webkit2-4.0
 
               # Start Virtual X server
               echo "Start X server..."
@@ -206,36 +219,41 @@ jobs:
               echo "Start window manager..."
               DISPLAY=:99 blackbox &
               sleep 1
-
             briefcase-run-prefix: 'DISPLAY=:99'
-            setup-python: false  # Use the system Python packages.
-            app-user-data-path: $HOME/.local/share/testbed
+            setup-python: false  # Use the system Python packages
+            app-user-data-path: "$HOME/.local/share/testbed"
 
           - backend: "windows"
             platform: "windows"
             runs-on: "windows-latest"
-            app-user-data-path: $HOME\AppData\Local\Tiberius Yak\Toga Testbed\Data
+            app-user-data-path: '$HOME\AppData\Local\Tiberius Yak\Toga Testbed\Data'
 
           - backend: "iOS"
             platform: "iOS"
             runs-on: "macos-14"
-            briefcase-run-args: ' -d "iPhone SE (3rd generation)"'
-            app-user-data-path: $(xcrun simctl get_app_container booted org.beeware.toga.testbed data)/Documents
+            briefcase-run-args: "--device 'iPhone SE (3rd generation)'"
+            app-user-data-path: "$(xcrun simctl get_app_container booted org.beeware.toga.testbed data)/Documents"
 
           - backend: "android"
             platform: "android"
             runs-on: "ubuntu-latest"
-            briefcase-run-args: " -d '{\"avd\":\"beePhone\",\"skin\":\"pixel_3a\"}' --Xemulator=-no-window --Xemulator=-no-snapshot --Xemulator=-no-audio --Xemulator=-no-boot-anim --shutdown-on-exit"
+            briefcase-run-prefix: JAVA_HOME=${JAVA_HOME_17_X64}
+            briefcase-run-args: >
+              --device '{"avd":"beePhone","skin":"pixel_3a"}'
+              --Xemulator=-no-window
+              --Xemulator=-no-snapshot
+              --Xemulator=-no-audio
+              --Xemulator=-no-boot-anim
+              --shutdown-on-exit
             pre-command: |
-              # check if virtualization is supported...
-              sudo apt install -qq --no-install-recommends cpu-checker coreutils && echo "CPUs=$(nproc --all)" && kvm-ok
               # allow access to KVM to run the emulator
               echo 'KERNEL=="kvm", GROUP="kvm", MODE="0666", OPTIONS+="static_node=kvm"' \
                 | sudo tee /etc/udev/rules.d/99-kvm4all.rules
               sudo udevadm control --reload-rules
               sudo udevadm trigger --name-match=kvm
     steps:
-    - uses: actions/checkout@v4.1.1
+    - name: Checkout
+      uses: actions/checkout@v4.1.1
       with:

@rmartin16
Copy link
Member Author

Soo, a bit of a fundamental issue with trying to run multiple tox -e py commands in parallel is they all try to build the Toga core and dummy packages in their build directory....and we can't have several independent processes trying to use these directories at the same time...

So, I guess either setuptools needs to be told to use a different directory for each install....or we need a different install strategy to avoid these in-tree builds....I'm not sure how to do either right now...

@rmartin16
Copy link
Member Author

rmartin16 commented Mar 4, 2024

So, setuptools is "great" when it works and an absolute PITA if you want to understand how to change something. Best I can tell, this whole "in-tree build" with setuptools is not configurable....at least with any incantation I found. That even included trying --config-setting=--build-option=--bdist-dir={env_tmp_dir}/build (which is an amazing setting in its own right) but only caused setuptools to do the build there....it still created the core/build directory and then copied everything to {env_tmp_dir}/build....

I finally discovered that just doing an editable install will avoid using the core/build directory and running tox p on Windows works now. (Without the editable installs of core and dummy, tox p always failed on Windows.)

@rmartin16
Copy link
Member Author

rmartin16 commented Mar 4, 2024

ugh....a different Windows error from tox p...conflicts when multiple processes are trying to move the same coverage database

________________________________ test_simple_as_module ________________________________

    def test_simple_as_module():
        """When a simple apps is started as `python -m app` inside a runnable module, the
        app path is the folder holding app.py."""
        # Spawn the simple testbed app using `-m app`
        cwd = Path(__file__).parent / "testbed/simple"
>       output = run_app(["-m", "app"], cwd=cwd)

tests\test_paths.py:81:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

args = ['-m', 'app'], cwd = WindowsPath('C:/Users/user/github/beeware/toga/core/tests/testbed/simple')

    def run_app(args, cwd):
        """Run a Toga app as a subprocess with coverage enabled and the Toga Dummy backend."""
        # We need to do a full copy of the environment, then add our extra bits;
        # if we don't the Windows interpreter won't inherit SYSTEMROOT
        env = os.environ.copy()
        env.update(
            {
                "COVERAGE_PROCESS_START": str(
                    Path(__file__).parent.parent / "pyproject.toml"
                ),
                "PYTHONPATH": str(Path(__file__).parent / "testbed/customize"),
                "TOGA_BACKEND": "toga_dummy",
            }
        )
        output = subprocess.check_output(
            [sys.executable] + args,
            cwd=cwd,
            env=env,
            text=True,
        )
        # When called as a subprocess, coverage drops its coverage report in CWD.
        # Move it to the project root for combination with the main test report.
        for file in cwd.glob(".coverage*"):
>           os.rename(file, Path(__file__).parent.parent / file.name)
E           PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\user\\github\\beeware\\toga\\core\\tests\\testbed\\simple\\.coverage.vm-win11.2836.XVrzunjx' -> 'C:\\Users\\user\\github\\beeware\\toga\\core\\.coverage.vm-win11.2836.XVrzunjx'

tests\test_paths.py:32: PermissionError

@rmartin16
Copy link
Member Author

It seems clear at this point running these tox environments in parallel should be considered unsupported; so, I just removed any recommendation to try to do that. Nonetheless, just running tox will still run the test suite for all Python versions sequentially. On Linux, the ext4 filesystem is a bit more tolerant of all this file clobbering...so, running in parallel does mostly work there.

There are two primary issues with blocking support for running in parallel:

  • Installing the required Toga packages
    • As answered in my question to the tox team, tox can only install one package for an environment
    • As detailed there, a new tox plugin could provide the ability to install multiple packages
    • Or env commands could build the wheel manually and install them
      • This should avoid the file clobbering issues since the wheel builds are isolated
    • If we switch off of setuptools, the other packaging backends allow for out-of-tree builds
  • Consolidating the .coverage databases
    • The test suite will try to move the .coverage files from where they are created to a central location
    • When this is done by multiple processes, it can create conflicts when more than one process tries to move the same file
    • Perhaps ideally, this wouldn't be necessary at all and coverage.py could be instructed to either create the files where they are needed or to look to where they are created

@rmartin16 rmartin16 marked this pull request as ready for review March 4, 2024 19:45
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

These tox changes all make sense to me, AFAICT align with the basic approach in Briefcase (although there are obviously differences because of the differences in the projects).

The docstring and typo fixes are also welcome; however, from an administrative angle it would have been easier to review those separately (especially given how many of those changes there are, in conjunction with the complexity of the CI/tox config deltas). I'm happy to approve and merge this as is - it's just something to keep in mind for the future.

@freakboy3742 freakboy3742 merged commit fe8347e into beeware:main Mar 5, 2024
34 checks passed
@rmartin16 rmartin16 deleted the tox-pytest-cov branch March 5, 2024 16:16
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.

None yet

2 participants