Skip to content

Set python args to be like CPython in CI #8181

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.

Sign up for GitHub

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

Merged
youknowone merged 5 commits into from
Jul 6, 2026
Merged

Set python args to be like CPython in CI #8181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bd809ce
Set python args to be like CPython in CI (ish)
ShaharNaveh Jun 26, 2026
128cee9
Use `--slow-ci`
ShaharNaveh Jun 27, 2026
74aca0c
`--dont-add-python-opts`
ShaharNaveh Jun 27, 2026
7b47348
unmark passing test
ShaharNaveh Jun 27, 2026
674bfa3
Merge remote-tracking branch 'upstream/main' into ci-cpython-like-args
ShaharNaveh Jul 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
CARGO_PROFILE_RELEASE_DEBUG: 0
CARGO_TERM_COLOR: always
CI: true
FORCE_COLOR: 1

jobs:
determine_changes:
Expand Down Expand Up @@ -294,20 +295,26 @@
- os: macos-latest
extra_test_args:
- '-u all'
- '--timeout 600'

@youknowone youknowone Jul 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if everything requires same args, dose it still need to be extra_test_args?

@ShaharNaveh ShaharNaveh Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it's repeated multiple times, I think this is the easiest way to manage it for now.

we repeat this arg in 2/3 times that we run the test suite. I can "hardcode" it in all other places instead, not a big deal

@youknowone youknowone Jul 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

extra_test_args changes the test name. then merge queue settings also must be updated.
that's not a big deal when it occasionally changes. but it happens more than common these days. So i wish that's still an argument, but not being a part of job title.

also it makes the CI job name unnecessary longer, not a big deal itself too though.

@ShaharNaveh ShaharNaveh Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

extra_test_args changes the test name. then merge queue settings also must be updated. that's not a big deal when it occasionally changes. but it happens more than common these days. So i wish that's still an argument, but not being a part of job title.

also it makes the CI job name unnecessary longer, not a big deal itself too though.

CPython uses an external file to manage their CI commands, we could do the same using just for example.

bevy wrote their own tool for that, we could do something similar.

I'm in favor of starting with the just approach (or similar), if you think it's a good idea.

There's another option which I'm less of a fan of, but still worth mentioning. cargo alias can also do that, but that's adding another layer without the full features that justgives us, i.e. reuse of variables. (we can't define a variable called common_args and use it everywhere).

lmk if that makes sense to you and/or worth implementing

@youknowone youknowone Jul 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

either way i am good as long as contributors still can copy the same CI command from web CI view to their terminal

- '--dont-add-python-opts'
env_polluting_tests:
- test_set
skips: []
timeout: 50
- os: ubuntu-latest
extra_test_args:
- '-u all'
- '--timeout 600'
- '--dont-add-python-opts'
env_polluting_tests:
- test_set
skips: []
timeout: 60
- os: windows-2025
extra_test_args:
- '-u all'
- '--timeout 600'
- '--dont-add-python-opts'
env_polluting_tests:
- test_set
skips: []
Expand Down Expand Up @@ -356,10 +363,10 @@
run: |
cores=$(python -c 'print(__import__("os").process_cpu_count())')
echo "cores=${cores}" >> "$GITHUB_OUTPUT"

- name: Run CPython tests
run: |
target/release/rustpython -m test -j ${{ steps.cores.outputs.cores }} ${{ join(matrix.extra_test_args, ' ') }} --slowest --fail-env-changed --timeout 600 -v -x ${{ env.FLAKY_MP_TESTS }} ${{ join(matrix.skips, ' ') }}
target/release/rustpython -u -m test --slow-ci -j ${{ steps.cores.outputs.cores }} ${{ join(matrix.extra_test_args, ' ') }} -x ${{ env.FLAKY_MP_TESTS }} ${{ join(matrix.skips, ' ') }}
timeout-minutes: ${{ matrix.timeout }}
env:
RUSTPYTHON_SKIP_ENV_POLLUTERS: true
Expand All @@ -370,7 +377,7 @@
echo "::group::Attempt ${attempt}"

set +e
target/release/rustpython -m test -j 1 ${{ join(matrix.extra_test_args, ' ') }} --slowest --fail-env-changed --timeout 600 -v ${{ env.FLAKY_MP_TESTS }}
target/release/rustpython -u -m test --slow-ci -j 1 ${{ join(matrix.extra_test_args, ' ') }} ${{ env.FLAKY_MP_TESTS }}
status=$?
set -e

Expand All @@ -395,7 +402,7 @@
for thing in "${target_array[@]}"; do
for i in $(seq 1 10); do
set +e
target/release/rustpython -m test -j 1 --slowest --fail-env-changed --timeout 600 -v "${thing}"
target/release/rustpython -u -m test --slow-ci -u all -j 1 --timeout 600 --dont-add-python-opts "${thing}"
exit_code=$?
set -e
if [ "${exit_code}" -eq 3 ]; then
Expand Down
1 change: 0 additions & 1 deletion Lib/test/test_future_stmt/test_future.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,6 @@ def test_unicode_literals_exec(self):
exec("from __future__ import unicode_literals; x = ''", {}, scope)
self.assertIsInstance(scope["x"], str)

@unittest.expectedFailure # TODO: RUSTPYTHON; barry_as_FLUFL (<> operator) not supported

@ShaharNaveh ShaharNaveh Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It seems that adding --dont-add-python-opts makes this pass 😟

@doma17 doma17 Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, that does seem a bit suspicious. Any idea why?

@ShaharNaveh ShaharNaveh Jul 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So I've now debugged it, it seems that it's actually because of the FORCE_COLOR=1 environment variable 🤦‍♂️

IMO the fix should be done over CPython side to add @support.force_not_colorized on the test method

@ShaharNaveh ShaharNaveh Jul 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

python/cpython#152749 contains a bit more more information about why

def test_syntactical_future_repl(self):
p = spawn_python('-i')
p.stdin.write(b"from __future__ import barry_as_FLUFL\n")
Expand Down
Loading