Skip to content

Add a --parallel-live to tox invocations in CI #2190

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

Jump to bottom
Merged
merged 1 commit into from
Jun 19, 2025
Merged

Add a --parallel-live to tox invocations in CI #2190

merged 1 commit into from
Jun 19, 2025
+26 −2

Conversation

webknjaz
Copy link
Member

This is a workaround for the upstream tox regression [1] when the previous recommendation of setting the TOX_PARALLEL_NO_SPINNER=1 env var conflicts with it auto-enabling parallelism.

Contributor checklist
  • Included tests for the changes.
  • PR title is short, clear, and ready to be included in the user-facing changelog.
Maintainer checklist
  • Verified one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@webknjaz webknjaz requested review from sirosen and Copilot June 19, 2025 14:04
@webknjaz webknjaz added the skip-changelog Avoid listing in changelog label Jun 19, 2025
Copilot

This comment was marked as outdated.

Sign in to view

@webknjaz webknjaz force-pushed the maintenance/tox-4.12-parallel-no-spinner-workaround branch from f4515e5 to ea9059b Compare June 19, 2025 14:11
@webknjaz webknjaz changed the title Add a -p auto to tox invocations in CI Add a --parallel-live to tox invocations in CI Jun 19, 2025
@webknjaz webknjaz force-pushed the maintenance/tox-4.12-parallel-no-spinner-workaround branch from ea9059b to f10da6f Compare June 19, 2025 14:14
@webknjaz webknjaz requested a review from Copilot June 19, 2025 14:14
Copilot
Copilot AI reviewed Jun 19, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses a regression in tox by adding the “--parallel-live” flag to various tox invocations within the CI configuration. Key changes include:

  • Adding “--parallel-live” to the “Prepare test environment” steps.
  • Updating tox invocations in both the installation and testing steps.
  • Adding explanatory comments referencing the upstream tox issue.

.github/workflows/ci.yml
@@ -107,9 +107,21 @@ jobs:
- name: Install test dependencies
run: python -m pip install -U tox virtualenv
- name: Prepare test environment
# NOTE: `--parallel-live` is a workaround for the regression in
Copy link
Preview

Copilot AI Jun 19, 2025

Choose a reason for hiding this comment

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

[nitpick] The workaround comment is repeated across multiple steps. Consider defining a YAML anchor or a shared comment block to improve maintainability and reduce duplication if future updates are needed.

Copilot uses AI. Check for mistakes.

Copy link
Member

Choose a reason for hiding this comment

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

Did we opt in to these? I find the copilot review -- in this case at least -- useless and distracting. I'll open an issue for us to discuss repo settings for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sirosen yes, I added it as a reviewer in this PR to see what it does. Sometimes LLMs surface interesting things, sometimes they hallucinate. But in general, it's interesting to see what it outputs and judge later.

@webknjaz webknjaz mentioned this pull request Jun 19, 2025
Merged
4 tasks
sirosen
sirosen approved these changes Jun 19, 2025
View reviewed changes
Copy link
Member

@sirosen sirosen left a comment

Choose a reason for hiding this comment

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

I'm also wondering if we should be using tox run as our invocation, but we can consider that later. I think this is a good fix as it is.

@webknjaz webknjaz enabled auto-merge June 19, 2025 18:57
@webknjaz
Add a --parallel-live to tox invocations in CI
35d7e70 
This is a workaround for the upstream tox regression [[1]] when the
previous recommendation of setting the `TOX_PARALLEL_NO_SPINNER=1` env
var conflicts with it auto-enabling parallelism.

[1]: tox-dev/tox#3193
@webknjaz webknjaz force-pushed the maintenance/tox-4.12-parallel-no-spinner-workaround branch from f10da6f to 35d7e70 Compare June 19, 2025 18:59
@webknjaz webknjaz added this pull request to the merge queue Jun 19, 2025
Merged via the queue into jazzband:main with commit 4992fa2 Jun 19, 2025
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sirosen sirosen sirosen approved these changes

Copilot code review Copilot Copilot left review comments

Assignees
No one assigned
Labels
skip-changelog Avoid listing in changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@webknjaz @sirosen