nesquena / rabl

General ruby templating with json, bson, xml, plist and msgpack support
http://blog.codepath.com/2011/06/27/building-a-platform-api-on-rails/
MIT License
3.65k stars 335 forks source link

rabl tests failing with rr 3.x and Ruby 3 #745

Closed dleidert closed 2 years ago

dleidert commented 2 years ago

Hello,

we have shipped a Debian package of the rabl gem for quite some years. Lately, we had to update the rr gem to version 3.0.7 and with this version the rabl tests fail in Ruby 3.0. I'm not yet quite sure which of the two gems is causing that, but I was wondering if you would mind taking a look at it?

Here is the build log: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=996350#5

tagliala commented 2 years ago

Hi,

I think the answer is in the bug itself

I was examining the problem. The problem seems to be ruby-rr 3.x. rabl actually requires rr 1.x, and with this version it works. I verified this by building against ruby-rr from Stable. But with ruby-rr 3.x in Testing/Unstable it fails.

https://github.com/nesquena/rabl/blob/855040f082920a85b6ced9388959043bcd46faa2/rabl.gemspec#L27

We can try to relax the dependency and allow 3.0. As far as I can tell, it requires Ruby >= 0 so it should still work on 1.9.3, ~the minimum supported version~ I think Ruby 1.8 is still supported but untested. There is no change to production code in the fix

The problem is that we should at first move the CI builds to GitHub Actions. I can take care of this but I can't promise

dleidert commented 2 years ago

Thanks for your response. As a downstream Linux distribution, we cannot ship more than one version of a gem. Thus, we have to use rr 3 now. I'd really appreciate your efforts. I saw a lot of ruby projects already using github actions lately. I think I can send you some examples, where the project has already migrated to github actions.

In the meantime, if we come up with a possible patch, we'll of course send it your way.

tagliala commented 2 years ago

Hi, I think I've fixed the tests so they pass against Ruby 3 and rr 3

You can take a try this branch: https://github.com/nesquena/rabl/tree/chore/update-to-rr-3

will not merge to master before #746 and a PR review on #748

dleidert commented 2 years ago

Thanks. BTW: Here is a possible template for your github actions: https://github.com/dtao/safe_yaml/compare/master...paolobrasolin:development

dleidert commented 2 years ago

Unfortunately, the tests still fail. This is what is reported:

ERROR
 Rabl::Engine with xml defaults #collection asserts that it sets object to be casted as a simple array => undefined method `to_xml' for [{}, {}]:Array
Did you mean?  to_yaml occured
 at /usr/lib/ruby/vendor_ruby/riot.rb:129:in `block in <module:Riot>'
at /usr/lib/ruby/vendor_ruby/riot.rb:38:in `run'
at /usr/lib/ruby/vendor_ruby/riot/reporter.rb:46:in `summarize'
at /usr/lib/ruby/vendor_ruby/riot.rb:39:in `block in run'
at /usr/lib/ruby/vendor_ruby/riot.rb:39:in `each'
at /usr/lib/ruby/vendor_ruby/riot.rb:39:in `block (2 levels) in run'
at /usr/lib/ruby/vendor_ruby/riot/context.rb:98:in `run'
at /usr/lib/ruby/vendor_ruby/riot/context.rb:146:in `run_sub_contexts'
at /usr/lib/ruby/vendor_ruby/riot/context.rb:146:in `each'
at /usr/lib/ruby/vendor_ruby/riot/context.rb:146:in `block in run_sub_contexts'
at /usr/lib/ruby/vendor_ruby/riot/context.rb:98:in `run'
at /usr/lib/ruby/vendor_ruby/riot/context.rb:146:in `run_sub_contexts'
at /usr/lib/ruby/vendor_ruby/riot/context.rb:146:in `each'
at /usr/lib/ruby/vendor_ruby/riot/context.rb:146:in `block in run_sub_contexts'
at /usr/lib/ruby/vendor_ruby/riot/context.rb:97:in `run'
at /usr/lib/ruby/vendor_ruby/riot/context.rb:108:in `local_run'
at /usr/lib/ruby/vendor_ruby/riot/context.rb:108:in `each'
at /usr/lib/ruby/vendor_ruby/riot/context.rb:109:in `block in local_run'
at /usr/lib/ruby/vendor_ruby/riot/rr.rb:33:in `run'
at /usr/lib/ruby/vendor_ruby/riot/assertion.rb:51:in `run'
at /usr/lib/ruby/vendor_ruby/riot/situation.rb:61:in `evaluate'
at /usr/lib/ruby/vendor_ruby/riot/situation.rb:61:in `instance_eval'
at /build/ruby-rabl-lLsWw7/ruby-rabl-0.14.5/test/xml_test.rb:48:in `block (4 levels) in <top (required)>'
at /usr/lib/ruby/vendor_ruby/tilt/template.rb:109:in `render'
at /build/ruby-rabl-lLsWw7/ruby-rabl-0.14.5/lib/rabl/template.rb:15:in `evaluate'
at /build/ruby-rabl-lLsWw7/ruby-rabl-0.14.5/lib/rabl/engine.rb:50:in `render'
at /build/ruby-rabl-lLsWw7/ruby-rabl-0.14.5/lib/rabl/engine.rb:386:in `cache_results'
at /build/ruby-rabl-lLsWw7/ruby-rabl-0.14.5/lib/rabl/engine.rb:51:in `block in render'
at /build/ruby-rabl-lLsWw7/ruby-rabl-0.14.5/lib/rabl/engine.rb:146:in `to_xml'

I have all your patches from the master branch applied here, also the rr fix. I'll dig into that again.

Edit: I see this with Ruby 2.7 already. We have some other differences like i18n > 1 (which shouldn't be a problem), and activesupport >= 6.1.4 (6.0.x had unresolved issues with Ruby 3.0).

tagliala commented 2 years ago

I can't reproduce the failure with Ruby 3.0.3 on GitHub Actions and in my dev machine: https://github.com/nesquena/rabl/runs/4340727284?check_suite_focus=true

dleidert commented 2 years ago

After digging into this. You should be able to reproduce it by only running xml_test.rb like this:

diff --git a/Rakefile b/Rakefile
index 9699394..141ea59 100644
--- a/Rakefile
+++ b/Rakefile
@@ -6,7 +6,7 @@ Bundler::GemHelper.install_tasks
 require 'rake/testtask'
 Rake::TestTask.new(:test) do |test|
   test.libs << 'lib' << 'test'
-  test.pattern = 'test/*_test.rb'
+  test.pattern = 'test/xml_test.rb'
   test.warning = true
   test.verbose = true
   if RUBY_VERSION < "1.9.0"

You should see the same errors. With the wildcard, the issue doesn't appear. I "fixed" our tests by loading your Rakefile and executing the test task defined there. With your rr patch, all tests succeeded.

tagliala commented 2 years ago

Thanks but I still can't reproduce. Since I'm on an Apple Silicon machine (with x64 ruby), I've also tried in a docker container using linux x64

$ uname -a
Linux docker 5.10.47-linuxkit #1 SMP PREEMPT Sat Jul 3 21:50:16 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
diff --git a/Rakefile b/Rakefile
index d1ba0fd..9cc600b 100644
--- a/Rakefile
+++ b/Rakefile
@@ -6,7 +6,7 @@ Bundler::GemHelper.install_tasks
 require 'rake/testtask'
 Rake::TestTask.new(:test) do |test|
   test.libs << 'lib' << 'test'
-  test.pattern = 'test/*_test.rb'
+  test.pattern = 'test/xml_test.rb'
   test.warning = true
   test.verbose = true
   if RUBY_VERSION < "1.9.0"
$ root@docker:/app/rabl# rake
/usr/share/rvm/rubies/ruby-3.0.3/bin/ruby -w -I"lib:lib:test" /usr/share/rvm/gems/ruby-3.0.3/gems/rake-13.0.6/lib/rake/rake_test_loader.rb "test/xml_test.rb" 
.........................
25 passes, 0 failures, 0 errors in 0.100037 seconds
dleidert commented 2 years ago

I also pinned all gem versions to the ones we use in Debian. If I'll find some time, I'll repeat the whole process and send you a complete diff. The error actually already happened with Ruby 2.7.

tagliala commented 2 years ago

Thanks. I was able to reproduce

We have some other differences like i18n > 1 (which shouldn't be a problem), and activesupport >= 6.1.4 (6.0.x had unresolved issues with Ruby 3.0).

I had to relax i18n dependency, and when AR/AS 6.1 is installed, then RABL does not work anymore.

I would like to track this issue separately

Ref: rails/rails@12959dee0f19a8e050dd1236b031c2c690729905

How to fix:

diff --git a/test/xml_test.rb b/test/xml_test.rb
index 9e8659a..3f72908 100644
--- a/test/xml_test.rb
+++ b/test/xml_test.rb
@@ -1,4 +1,6 @@
 require 'active_support/core_ext/hash/conversions'
+require 'active_support/time'
+
 require File.expand_path('../teststrap', __FILE__)
 require 'rabl/template'
tagliala commented 2 years ago

Hi @dleidert, since Rails 7 is approaching and rabl apparently needs a new fix, I've merged all the changes to non-production code and I hope that now all specs will pass against master branch

nesquena commented 2 years ago

Thanks again @tagliala for working on this fix