Skip to content

Make CI pass #262

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

Merged
merged 9 commits into from
Jul 29, 2024
Merged

Make CI pass #262

merged 9 commits into from
Jul 29, 2024

Conversation

gdalle
Copy link
Member

@gdalle gdalle commented Jul 29, 2024

  • Put all tests inside a big @testset
  • Identify and mark failing cat tests as broken

At the moment, testcat in ArrayFunctionTests.jl is broken for 4 or more arguments because of a method ambiguity (see #242). This PR doesn't fix the source of the problem, it just ignores broken tests so that CI passes. To be fair, these tests are not very useful.

These broken tests pass on Julia 1.0 and 1.3 (for which support should be dropped anyway) but fail on 1.10. At some point between the two, a type piracy appeared in SparseArrays which caused the ambiguity.

@gdalle gdalle changed the title Mark cat tests as broken for > 3 arguments (method ambiguity) Mark cat tests as broken for > 3 arguments Jul 29, 2024
@gdalle gdalle changed the title Mark cat tests as broken for > 3 arguments Make CI pass Jul 29, 2024
@gdalle gdalle merged commit 69fb150 into master Jul 29, 2024
5 checks passed
@gdalle gdalle deleted the gd/fix_ci branch July 29, 2024 14:56
Copy link
Member

@devmotion devmotion 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 appreciate "fixing" CI, I think this PR (and others merged recently) should have been reviewed.

@test x isa type
@test value(x) == f(args...; kwargs...)
broken = f == hcat && (args[2] isa AbstractMatrix)
if broken && VERSION >= v"1.4"
Copy link
Member

Choose a reason for hiding this comment

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

Why 1.4? AFAICT (and as mentioned eg in #259) this issue is caused by a type piracy in SparseArrays introduced in Julia 1.10 (see JuliaSparse/SparseArrays.jl#431).

Copy link
Member Author

@gdalle gdalle Jul 31, 2024

Choose a reason for hiding this comment

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

I didn't do the bisection to figure out which version of Julia was guilty. I just took the version that was above 1.3, on which I was sure the tests were passing. I can replace it with 1.10, no problem.
Tbh we should just drop 1.0 and 1.3, even Julia doesn't support those anymore

@gdalle
Copy link
Member Author

gdalle commented Jul 31, 2024

While I appreciate "fixing" CI, I think this PR (and others merged recently) should have been reviewed.

That's true, but the last issue I opened, outlining a genuine bug (#251) got zero answer in four months. I was unsure if anyone was actually watching or interested.

@gdalle
Copy link
Member Author

gdalle commented Jul 31, 2024

Another reason why this PR was necessary is that for lack of a @testset, tests were failing and exiting at the first failure. So having a systematic failure in the concatenation tests on 1.10 means we never would have caught a bug happening after that one.

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.

2 participants