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

Mark known test failures #2137

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Mark known test failures #2137

wants to merge 1 commit into from

Conversation

macro1
Copy link
Contributor

@macro1 macro1 commented Dec 2, 2024

Marks known test failures tied to #2131 as we do not have a strategy for remediating.

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).

@macro1 macro1 added ci Related to continuous integration tasks skip-changelog Avoid listing in changelog and removed ci Related to continuous integration tasks labels Dec 2, 2024
@macro1 macro1 marked this pull request as ready for review December 2, 2024 05:36
@macro1 macro1 requested a review from Mr-Sunglasses December 2, 2024 05:37
@webknjaz
Copy link
Member

webknjaz commented Dec 2, 2024

IIRC, the fix is #2106. I'd rather accept it instead.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

While I don't think that the tests should be suppressed at all, if we go this way, the places mentioned below must be fixed first.

tests/test_cli_compile.py Outdated Show resolved Hide resolved
tests/test_cli_compile.py Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@macro1
Copy link
Contributor Author

macro1 commented Dec 2, 2024

@webknjaz we either suppress or halt development on the project. there's no proposed solution for #2110 #2131

edit: 🤦 i tied everything to the wrong issue. i meant #2131

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

we either suppress or halt development on the project. there's no proposed solution for #2131

I guess you could work on the solution to that issue with a few temporary commits that unblock the CI for you and could be dropped later.

Meanwhile, the correct solution to the failing tests is #2106. Fixing it is what has to happen first.

I'm blocking this because the change at https://github.com/jazzband/pip-tools/pull/2137/files#r1867006850 is harmful in that it contributes to shoving the problem under the rug, which is a big deal.

Since nobody seems to be attempting to look into completing #2106, I'll try to check it myself later today.

@macro1
Copy link
Contributor Author

macro1 commented Dec 3, 2024

alright. so we'll rely on #2106 for the wheel expected in output issue, #2134 for the pre-commit issue (the mypy step no longer supports 3.8 runtime). in this PR, i'll narrow it down to the known issue introduced in pip 24.3

i'm curious about #2106 and the problems we would potentially be shoving beneath the rug, so i may comment there later when i have more time

@macro1 macro1 changed the title Adjust CI Mark known test failures Dec 3, 2024
@macro1 macro1 enabled auto-merge December 3, 2024 06:06
@macro1 macro1 requested a review from webknjaz December 3, 2024 06:07
@webknjaz webknjaz marked this pull request as draft December 4, 2024 03:15
auto-merge was automatically disabled December 4, 2024 03:15

Pull request was converted to draft

@webknjaz
Copy link
Member

webknjaz commented Dec 4, 2024

I've converted this to draft to prevent accidental merging ahead of time.

@macro1
Copy link
Contributor Author

macro1 commented Dec 4, 2024

@webknjaz currently this PR is blocked by your request for changes, and now it is also a draft, so i don't think it's going anywhere

what needs to be merged ahead of this PR? the changes have been reduced to marking specific tests as expected failures when using a pip version of 24.3 or higher, which will remain true till there's a solution for #2131

@webknjaz
Copy link
Member

webknjaz commented Dec 4, 2024

@macro1 in #2106, I'm experimenting with making the testing more predictable by pinning pip more aggressively in the test infra (#2106 (comment)). I recall that PR failing weirdly on Windows, and now that's not exactly the case.

Now that I identified this, I'm going to see if I can change that setup together with #2134 and make pip updates in CI more controlled.

@webknjaz webknjaz self-assigned this Dec 4, 2024
@webknjaz
Copy link
Member

@macro1 this is not technically necessary anymore. I've still clicked “rebase” to get the CI statuses to me green. Though, perhaps this should be updated together with bumping the pip version that I pinned in another PR. And with the corresponding fixes instead of disabling the tests...

@webknjaz webknjaz marked this pull request as ready for review December 16, 2024 04:41
@webknjaz
Copy link
Member

@macro1 let me know if you think this should be merged anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog Avoid listing in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants