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

Update to rails 7.2.0 #5070

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

tomhughes
Copy link
Member

No description provided.

@tomhughes
Copy link
Member Author

Looks like this is a no-go until we can move to ruby 3.1 then...

@tomhughes tomhughes force-pushed the rails72 branch 4 times, most recently from 97591e4 to 5e4091d Compare August 14, 2024 20:03
@mmd-osm
Copy link
Collaborator

mmd-osm commented Aug 22, 2024

I did a smoke test on Ubuntu 22.04 + ruby 3.3.2 (2024-05-30 revision e5a195edf6), which I installed manually at one point. Tests are passing without issue:

Finished in 66.145466s, 26.2301 runs/s, 15520.4439 assertions/s.
1735 runs, 1026607 assertions, 0 failures, 0 errors, 0 skips

A few tests are reporting some warning "Test is missing assertions":

  • test_3rd_party_token: test/integration/client_applications_test.rb:73
  • test_body_class: test/helpers/application_helper_test.rb:92
  • test_header_nav_link_class: test/helpers/application_helper_test.rb:94
  • test_lost_password_recovery_success: test/integration/user_creation_test.rb:174
  • test_show_no_friends: test/controllers/dashboards_controller_test.rb:13

@tomhughes
Copy link
Member Author

Not sure why you're getting those messages because I don't see them.

I mean I can see what the problem is - people have written stubs for tests that don't do anything yet. It's just a bit odd that you get warned about them and I don't.

@tomhughes
Copy link
Member Author

In some cases they're not even stubs - a test of an action with no assertions still checks that a given action will run without throwing any exceptions.

@mmd-osm
Copy link
Collaborator

mmd-osm commented Aug 22, 2024

The messages also show up in GitHub action runs, such as: https://github.com/openstreetmap/openstreetmap-website/actions/runs/10394204557/job/28783478253?pr=5070 around line 70:

Test is missing assertions: `test_lost_password_recovery_success` /home/runner/work/openstreetmap-website/openstreetmap-website/test/integration/user_creation_test.rb:174
Test is missing assertions: `test_body_class` /home/runner/work/openstreetmap-website/openstreetmap-website/test/helpers/application_helper_test.rb:92
Test is missing assertions: `test_header_nav_link_class` /home/runner/work/openstreetmap-website/openstreetmap-website/test/helpers/application_helper_test.rb:94

@mmd-osm
Copy link
Collaborator

mmd-osm commented Aug 22, 2024

According to the docs, https://api.rubyonrails.org/v7.1.3.4/classes/ActiveSupport/Testing/Assertions.html#method-i-assert_nothing_raised should handle this case. Probably a bit redundant, as exceptions would cause the test to fail anyway.

@tomhughes
Copy link
Member Author

Yes I figured it out... I had RUBYOPT set to suppress warnings in my environment. I think at some point there were some annoying warnings we couldn't get rid of and rails doesn't normally use actual ruby warnings like that when it wants to warn about something.

@tomhughes tomhughes force-pushed the rails72 branch 2 times, most recently from 6eb2b88 to 1a937de Compare August 26, 2024 21:38
@gravitystorm
Copy link
Collaborator

I think it's a shame that we have to drop 22.04 support, since I expect some developers won't have yet upgraded their Ubuntu to 24.04. (I haven't, but I used rbenv - we don't want to rely on that for new developers).

It seems like an unfortunate combination of release dates and support periods:

  • ruby 3.0 was released in Dec 2020
  • ruby 3.1 was released in Dec 2021, which was in time for Ubuntu 22.04 but wasn't the chosen version
  • ruby 3.0 support ceased in Apr 2024
  • rails drops 3.0 support (and therefore Ubuntu 22.04 support) in Aug 2024

So we end up with a period of only 4 months where we have two versions of Ubuntu available. We'll face a similar problem in two year's time, because Ubuntu 24.04 shipped with ruby 3.2, which will be EOL in Mar 2026 i.e. just before Ubuntu 26.04 LTS. I wish either ruby would extend their support a little, or Ubuntu would use the newest ruby, or rails would be more tolerant of slightly EOL rubies, all the make this developer experience a bit smoother.

Nevertheless, I'm happy to move forward. I would say that we should:

  • Drop 22.04 from the testing matrix. If we can't run on real Ubuntu 22.04, there's little point in testing on Github Flavoured Ubuntu
  • Adjust the Vagrantfile to use ubuntu/noble64 or generic/ubuntu2404 as appropriate
  • There's a failure in building the docker image in CI

@tomhughes
Copy link
Member Author

There's an extra complication which is that we can't (yet) use the GitHub 24.04 image to run tests as it's still beta and is missing firefox (planned to be fixed before it leaves beta) so for now we probably have to stick with 22.04 for GHA.

@tomhughes
Copy link
Member Author

Also we're waiting on jumph4x/canonical-rails#92 before we can go to 7.2.1 which is already out.

@tomhughes tomhughes force-pushed the rails72 branch 2 times, most recently from d66be9d to e60faba Compare September 4, 2024 18:09
@tomhughes
Copy link
Member Author

tomhughes commented Sep 4, 2024

I've updated this as follows:

The docker issue is rubyjs/mini_racer#305 / rubyjs/libv8-node#36 which stops mini_racer installing on 24.04 currently.

@tomhughes
Copy link
Member Author

Oh just to add that all the production servers, and the dev server, are now on Debian 12 and hence ruby 3.1.

@mmd-osm
Copy link
Collaborator

mmd-osm commented Sep 4, 2024

#5010 (comment) proposes some changes to use Debian 12 for Docker. Maybe we should try to resolve the remaining issue there, to keep everything a bit more in sync.

Also add Ubuntu 24.04 and drop 20.04 as 24.04 is needed for
ruby 3.1 without rvm so we should test it.
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