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

Allow CI failures on Julia nightly builds #776

Merged
merged 1 commit into from
May 2, 2020

Conversation

c42f
Copy link
Member

@c42f c42f commented Apr 30, 2020

@c42f c42f merged commit 38f5e80 into master May 2, 2020
@c42f c42f deleted the cjf/allow-ci-nightly-failures branch May 2, 2020 05:51
@c42f
Copy link
Member Author

c42f commented May 2, 2020

Surprisingly, this seems to have worked first go...

@tkf
Copy link
Member

tkf commented May 3, 2020

Does it make sense to enable status check? https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks

Maybe use the "loose" one? https://help.github.com/en/github/administering-a-repository/types-of-required-status-checks

It's a bit hard to eyeball that only nightly is failing. It's also kind of nice to codify merge policy if it's not too much of a hustle.

@c42f
Copy link
Member Author

c42f commented May 11, 2020

Well I think everyone has been pretty careful about merging, but I enabled some of these required status checks; let's see how they go.

@tkf
Copy link
Member

tkf commented May 11, 2020

Maybe I should've clarified the motivation. I was bringing it up as I wasn't sure if I could merge #780 myself until I found this PR. But now that I know the policy (breaking nightly is OK). Anyway, thanks for tweaking the setting.

@c42f
Copy link
Member Author

c42f commented May 12, 2020

Breaking nightly isn't exactly ok :-/

The situation here is that nightly broke us, but the underlying issue in Base is extremely hairy (@allocated doesn't work as expected and figuring out why is hard) so this is a stopgap measure so that CI doesn't fail on all StaticArrays PRs in the meantime.

I guess the right fix for us (absent a fix in Base) would be to rip out all use of @allocated from our tests. But that's also unfortunate.

@tkf
Copy link
Member

tkf commented May 12, 2020

Ah, I see. Maybe we can hide it in a VERSION check? Something like

@test_broken_if VERSION >= v"1.5-" @allocated(...) == 0

with

macro test_broken_if(cond, ex)
    quote
        if $cond
            $Test.@test_broken $ex
        else
            $Test.@test $ex
        end
    end |> esc
end

?

This way, we can still get the feedback from Julia 1.0 etc. when we have some performance regression.

@c42f
Copy link
Member Author

c42f commented May 12, 2020

Sounds reasonable to me. Hmm, maybe upstream fixed this... oddly enough all tests at #783 seem to have passed.

@tkf
Copy link
Member

tkf commented May 12, 2020

That's good news :)

@c42f
Copy link
Member Author

c42f commented May 12, 2020

Actually it looks like @mateuszbaran may have just worked around the new nightly failures? (Thanks!)

@mateuszbaran
Copy link
Collaborator

Yes, I have done that: https://github.com/JuliaArrays/StaticArrays.jl/pull/783/files#diff-38c102d503c0e0a7f253cec577d6f26dR80 but @test_broken_if looks like a better idea.

@tkf
Copy link
Member

tkf commented May 12, 2020

@mateuszbaran Thanks a lot for fixing this. @test_broken_if doesn't compose well with @test_noalloc so I think it's good as-is.

(One of the not-so-great parts of Test is that @test_broken and @test_skip don't compose well...)

@c42f
Copy link
Member Author

c42f commented May 14, 2020

Agreed. And @inferred has the opposite problem: it composes with @test, but can't be neatly used outside @test because throwing an exception outside @test will short circuit the entire testset.

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.

3 participants