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

Test against rails 8.0 #3702

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

Conversation

jewilmeer
Copy link

As rails 8 came out, and not a lot has changed internally since 7.2, it might just work :-)

platforms :ruby, :mswin, :mingw, :x64_mingw do
gem "mysql2", ">= 0.3.14"
gem "pg", ">= 1.0.0"
gem "sqlite3", "~> 1.3"

Choose a reason for hiding this comment

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

https://github.com/railsadminteam/rails_admin/actions/runs/11738544873/job/32701267375?pr=3702#step:7:30

From Rails 8.0, it looks like the version of sqlite3 needs to be upgraded to v2.1 or higher.

Copy link
Author

Choose a reason for hiding this comment

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

indeed, I upgraded it, and now it seems to work

@jewilmeer
Copy link
Author

It looks like a lot of unrelated things are broken, not sure how to proceed.

@dsinn
Copy link

dsinn commented Nov 15, 2024

@jewilmeer Indeed, I was able to reproduce the issue on master. I suspect that because the json gem isn't pinned, the latest CI runs install a version that is greater than 2.7.6, the newer versions' JSON.pretty_generate({}) behaves differently, as it no longer contains a newline. Compare:

Old behaviourNew behaviour
$ irb
irb(main):001> require "json"
=> true
irb(main):002> JSON::VERSION
=> "2.7.6"
irb(main):003> puts JSON.pretty_generate({})
{
}
=> nil
$ irb
irb(main):001> require "json"
=> true
irb(main):002> JSON::VERSION
=> "2.8.2"
irb(main):003> puts JSON.pretty_generate({})
{}
=> nil

The test fails because RailsAdmin's Json field class calls JSON.pretty_generate, and the test's regex is expecting at least one newline from its result. As a quick test, I made the following change, and the tests pass:

diff --git a/spec/rails_admin/config/fields/types/json_spec.rb b/spec/rails_admin/config/fields/types/json_spec.rb
index a1e03958..a749d930 100644
--- a/spec/rails_admin/config/fields/types/json_spec.rb
+++ b/spec/rails_admin/config/fields/types/json_spec.rb
@@ -24,7 +24,7 @@ RSpec.describe RailsAdmin::Config::Fields::Types::Json do
     it 'returns correct value for empty json' do
       allow(object).to receive(:json_field) { {} }
       actual = field.with(bindings).formatted_value
-      expect(actual).to match(/{\n+}/)
+      expect(actual).to match(/{\n*}/)
     end
 
     it 'retuns correct value' do
@@ -72,7 +72,7 @@ RSpec.describe RailsAdmin::Config::Fields::Types::Json do
     it 'returns correct value for empty json' do
       allow(object).to receive(:json_field) { {} }
       actual = field.with(bindings).export_value
-      expect(actual).to match(/{\n+}/)
+      expect(actual).to match(/{\n*}/)
     end
 
     it 'returns correct value' do

I don't see any need for the newline, so I'd lean towards just updating the test (whether to split the change into a separate PR is up to you and the maintainers), but I'm also not a frequent user of RailsAdmin.

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