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

Make CI green by skipping failing ruby-head and jruby-head tests #1162

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

Conversation

johnnyshields
Copy link

No description provided.

@albus522
Copy link
Member

What are you fixing?

@johnnyshields
Copy link
Author

johnnyshields commented Jan 18, 2022

@albus522 the CI on master is not currently green. This PR aims to fix that.

It seems that JRuby 9.3 is the source of failure...

@albus522
Copy link
Member

It will green up when jruby supports Rails 7. It is acceptably red for now.

@johnnyshields
Copy link
Author

@albus522 the issues aren't related to Rails 7.0.0, as you can see it's failing on Rails 6.1.0 too.

It's this bug: jruby/jruby#6984

@johnnyshields
Copy link
Author

I would suggest to merge this PR as it makes failures much more clear than what's on currently on DJ master. We should ignore failures on head etc. which master is not currently doing properly.

@albus522
Copy link
Member

albus522 commented Jan 18, 2022

This isn't fixing anything. It is just removing a bunch of stuff that is temporarily failing.

Moving the the continue-on-error into the steps is not an accurate move. If any of the steps fail, the next isn't going to work.

You can see the continue-on-error working here https://github.com/collectiveidea/delayed_job/actions/runs/411480881

It is not green right now because jruby does not register as >= 2.7.0 which is required by Active Support 7.

@johnnyshields
Copy link
Author

johnnyshields commented Jan 18, 2022

@albus522 in the link you just sent, jruby-head and ruby-head are both appearing red (screenshot below). They should be green (i.e. skipped). -head tests are always very brittle and if we allow them to be red (non-skipped) then they be red for everyone raising a PR, i.e. the next time they break (which is almost guaranteed.)

I've updated the title of the PR to more accurately reflect the change here.

image

@johnnyshields johnnyshields changed the title Fix CI failures Fix CI failures by skipping failing ruby-head and jruby-head tests Jan 18, 2022
@johnnyshields johnnyshields changed the title Fix CI failures by skipping failing ruby-head and jruby-head tests Make CI green by skipping failing ruby-head and jruby-head tests Jan 18, 2022
@albus522
Copy link
Member

I do not agree, failures should be red. The build finish step knows if the suite is considered passing. I agree that Github could make it more obvious and better distinguish allowed failures vs hard failures, but I will not be artificially marking everything green to get around a Github UI deficiency.

@johnnyshields
Copy link
Author

johnnyshields commented Jan 18, 2022

Firstly, 100% agreed that Github should do a better job of marking skipped vs. failed (like Travis CI used to.) Unfortunately today Github does not.

IMHO CI should indicate whether the code is stable for all currently supported versions -- head / edge versions are not officially supported yet. Otherwise, the overall CI status is almost always going to show up as red, because one of Rails/Ruby/JRuby head / edge is pretty much always going to break somewhere.

@albus522
Copy link
Member

As I pointed out in the build above, the CI status is green. Some jobs are accurately red, but the overall build status is green
Screen Shot 2022-01-18 at 1 50 48 PM
.

@johnnyshields
Copy link
Author

Sure doesn't look that way to me on master

image

@albus522
Copy link
Member

Master isn't green right now.

And I see I botched the release commit message this round.

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