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

Pants-internal tests that use pytest.mark.platform_specific_behaviour aren't required to have corresponding target tag #21608

Open
huonw opened this issue Nov 4, 2024 · 0 comments
Labels
bug category:internal CI, fixes for not-yet-released features, etc.

Comments

@huonw
Copy link
Contributor

huonw commented Nov 4, 2024

Describe the bug

We should have some sort of linting (or other tooling) that makes it impossible to define "platform-specific behaviour" (PSB) tests incorrectly (forgetting to mark the test or forgetting to tag the target).

Pants has a bunch of tests marked as "platform-specific behaviour" (PSB), defining a high-value subset of tests for running on new platforms. (i.e. the cheap/free Linux x86-64 runners run all tests, and our other runners only run the PSB ones.)

Marking a test like this requires two steps, to coordinate with the ./pants --tag=+platform_specific_behavior test :: -- -m platform_specific_behavior invocation used to run them:

  1. apply pytest.mark.platform_specific_behavior to the test (within the ..._test.py file) so Pytest can select the test when running that file. Example:
    @pytest.mark.platform_specific_behavior
    @pytest.mark.parametrize(
    "major_minor_interpreter",
    all_major_minor_python_versions(Black.default_interpreter_constraints),
    )
    def test_passing(rule_runner: PythonRuleRunner, major_minor_interpreter: str) -> None:
  2. add platform_specific_behavior tag to the test's target (within a BUILD file) so Pants can select the files to run. Example:
    python_tests(
    name="tests",
    overrides={
    "rules_integration_test.py": {
    "tags": ["platform_specific_behavior"],
    "timeout": 240,
    }
    },
    )

It's easy to do step 1 without step 2.

This shell pipeline estimates these mistakes, by finding any *_test.py that mention platform_specific_behavior where the adjacent BUILD does not (and vice versa):

comm -3 \
  <(rg --files-with-matches 'platform_specific_behavior' --glob '*_test.py' . | sed -E 's|(.*)/[^/]+|\1|' | sort | uniq) \
  <(rg --files-with-matches 'platform_specific_behavior' --glob 'BUILD' . | sed -E 's|(.*)/BUILD|\1|' | sort | uniq)

(NB. this isn't perfect, would be better to examine pants peek output, e.g. if there's two *_test.py files that have the tag in a directory, the BUILD file should have both tests tagged with it.)

At the time of writing these tests are missing the BUILD file tag (fixed in #21609):

  • ./src/python/pants/backend/build_files/fmt/black/integration_test.py
  • ./src/python/pants/backend/build_files/fmt/ruff/integration_test.py
  • ./src/python/pants/backend/build_files/fmt/yapf/integration_test.py
  • ./src/python/pants/backend/sql/lint/sqlfluff/rules_integration_test.py

Pants version
main / 6f8e16e

OS
N/A
Additional info
N/A

@huonw huonw added bug category:internal CI, fixes for not-yet-released features, etc. labels Nov 4, 2024
huonw added a commit that referenced this issue Nov 4, 2024
)

This PR adds the `platform_specific_behavior` tag to the `python_test`
targets of 4 additional test files in this repo.

These test files use this repo's custom
`pytest.mark.platform_specific_behavior` mark to indicate tests are
high-value to test on non-Linux-x86_64 platforms. However, they were not
being run in CI because the testing invocation uses both Pants-level tag
and Pytest-level mark filtering: `./pants
--tag=+platform_specific_behavior test :: -- -m
platform_specific_behavior`.

This PR fixes the current problems identified in #21608, but doesn't
stop us from making the same mistakes in future (and thus doesn't close
that ticket).
@huonw huonw changed the title Some Pants-internal tests that use pytest.mark.platform_specific_behaviour aren't required to have corresponding target tag Pants-internal tests that use pytest.mark.platform_specific_behaviour aren't required to have corresponding target tag Nov 4, 2024
huonw added a commit that referenced this issue Nov 8, 2024
…haviour, test on macOS (#21610)

This goes through many backends and marks one test as
`platform_specific_behavior` to ensure the backends have some testing on
platforms other than x86-64 Linux (macOS, and arm64 Linux).

The particular value in doing this is validating the set-up code, e.g.
got the right configuration for installing/downloading the tool, rather
than validating the details of the behaviour. I think we generally
assume most tools behave similar across platforms, once they're up and
running.

This PR focuses on non-JVM and non-JS backends, meaning raw binaries
(`ExternalTool`m `TemplatedExternalTool`) and Python tools installed
from PyPI (`PythonToolBase`, `PythonToolRequirementsBase`). It
particularly focuses on the backends that don't require installing any
additional software in CI, see #21620 and #21621 for additional backends
that do require installing Go and Docker respectively.

The impression I get is the JVM and JS tools are more
platform-independent, often? For instance, their lockfiles/artifacts
don't mention specific platforms, unlike Python wheels.

I found these backends via the following shell command, plus other
exploration:

```shell
for file in $(rg --files-with-matches "class.*(PythonTool.*Base|ExternalTool)" --glob '*.py' . | sed 's@/[^/]*$@@' | sort | uniq); do
  rg -q "platform_specific_behavior" $file || echo $file
done
```

I may've missed some!

This increases the time to run tests in CI on the relevant platforms, by
slight more than 2×.

| platform     | before                 | after         |
|--------------|------------------------|---------------|
| Linux x86-64 | runs all tests always  | no change     |
| Linux arm64  | ~3:30                  | ~7:30         |
| macOS x86-64 | ~5:30                  | 12:00 - 13:30 |
| macOS arm64  | not run in CI (#20993) | no change     |

Related: #21608, #20193, #20993
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

No branches or pull requests

1 participant