Skip jr_jackson test unless jruby

[djberg96]

I noticed that if I ran rake locally the jr_jackson specs would fail because I wasn't running JRuby. This PR just modifies it to skip the specs unless it's JRuby, which is already done elsewhere within the specs.

There's also a whitespace trim.

Edit: I'm also skipping nsjsonserialization by default in the spec helper because, as per your own comments, it's a legacy parser of some sort. I am not even sure what it's for, as I could not find an "ok_json" gem.

[BuonOmo]

@djberg96 TBH I think this is redundancy: I think you have to decide on one and only one way to split between JRuby and Ruby impl. Historically it has been done with the SKIP_ADAPTERS method, I'd stay with it as it is not so bad. This is adding conditions that I would not want to have to deal with.

For a dev of the gem, it is then just a matter of setting SKIP_ADAPTERS=jr_jackson,nsjsonserialization,json_pure and either jruby or ruby specific ones in their shell env.

Same goes with skipping nsjsonserialization, I think having one way only to handle all of these is better. And if it is really so outdated, then we could delete it ^^

[djberg96]

@BuonOmo This was just a quick and dirty fix, I was only trying to stay within the framework that was already laid out. If it were up to me, this would all be refactored and handled with RSpec.configure + filter_run_excluding within spec_helper.rb and spec tags, not this ENV nonsense.

[BuonOmo]

Got you, but I still think there is no urge in doing this refactoring...

[djberg96]

@BuonOmo I'm afraid I don't agree, we should be able to run the tests locally and pass. Case in point, I currently see this locally when I run rake:

# ruby 3.2.1 (2023-02-08 revision 31819e82c8) [arm64-darwin22]
>rake
/Users/daniel.berger/.rbenv/versions/3.2.1/bin/ruby -I/Users/daniel.berger/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/rspec-support-3.13.1/lib:/Users/daniel.berger/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/rspec-core-3.13.0/lib /Users/daniel.berger/.rbenv/versions/3.2.1/lib/ruby/gems/3.2.0/gems/rspec-core-3.13.0/exe/rspec --pattern spec/\{multi_json,options_cache\}_spec.rb

Randomized with seed 36269
................FF...............

Failures:

  1) MultiJson aliases jrjackson allows jrjackson alias as string
     Got 1 failure and 1 other error:

     1.1) Failure/Error: expect { MultiJson.use 'jrjackson' }.not_to raise_error
          
            expected no Exception, got #<MultiJson::AdapterError: Did not recognize your adapter specification (cannot load such file -- jrjackson).> with backtrace:
              # ./lib/multi_json/adapters/jr_jackson.rb:1:in `<top (required)>'
              # ./lib/multi_json.rb:157:in `load_adapter_from_string_name'
              # ./lib/multi_json.rb:101:in `load_adapter'
              # ./lib/multi_json.rb:91:in `use'
              # ./spec/multi_json_spec.rb:187:in `block (5 levels) in <top (required)>'
              # ./spec/multi_json_spec.rb:187:in `block (4 levels) in <top (required)>'
          # ./spec/multi_json_spec.rb:187:in `block (4 levels) in <top (required)>'

     1.2) Failure/Error: after { expect(MultiJson.adapter).to eq(MultiJson::Adapters::JrJackson) }
          
          NameError:
            uninitialized constant MultiJson::Adapters::JrJackson
          # ./spec/multi_json_spec.rb:180:in `block (4 levels) in <top (required)>'

  2) MultiJson aliases jrjackson allows jrjackson alias as symbol
     Got 1 failure and 1 other error:

     2.1) Failure/Error: expect { MultiJson.use :jrjackson }.not_to raise_error
          
            expected no Exception, got #<MultiJson::AdapterError: Did not recognize your adapter specification (cannot load such file -- jrjackson).> with backtrace:
              # ./lib/multi_json/adapters/jr_jackson.rb:1:in `<top (required)>'
              # ./lib/multi_json.rb:157:in `load_adapter_from_string_name'
              # ./lib/multi_json.rb:101:in `load_adapter'
              # ./lib/multi_json.rb:91:in `use'
              # ./spec/multi_json_spec.rb:183:in `block (5 levels) in <top (required)>'
              # ./spec/multi_json_spec.rb:183:in `block (4 levels) in <top (required)>'
          # ./spec/multi_json_spec.rb:183:in `block (4 levels) in <top (required)>'

     2.2) Failure/Error: after { expect(MultiJson.adapter).to eq(MultiJson::Adapters::JrJackson) }
          
          NameError:
            uninitialized constant MultiJson::Adapters::JrJackson
          # ./spec/multi_json_spec.rb:180:in `block (4 levels) in <top (required)>'

Finished in 0.19847 seconds (files took 0.0965 seconds to load)
33 examples, 2 failures

Failed examples:

rspec ./spec/multi_json_spec.rb:186 # MultiJson aliases jrjackson allows jrjackson alias as string
rspec ./spec/multi_json_spec.rb:182 # MultiJson aliases jrjackson allows jrjackson alias as symbol

If I comment out the jr_jackson specs completely and re-run rake, I see the following:

Randomized with seed 22256
................................F...............

Failures:

  1) MultiJson::Adapters::JsonPure behaves like an adapter .dump dumps time in correct format
     Failure/Error: object.to_json(options)
     
     NoMethodError:
       undefined method `strict?' for {}:Hash
     Shared Example Group: "an adapter" called from ./spec/json_pure_adapter_spec.rb:10
     # ./lib/multi_json/adapters/json_common.rb:19:in `dump'
     # ./lib/multi_json/adapter.rb:25:in `dump'
     # ./lib/multi_json.rb:139:in `dump'
     # ./spec/shared/adapter.rb:61:in `block (3 levels) in <top (required)>'

Finished in 0.01486 seconds (files took 0.10861 seconds to load)
48 examples, 1 failure

Failed examples:

rspec './spec/json_pure_adapter_spec.rb[1:1:3:3]' # MultiJson::Adapters::JsonPure behaves like an adapter .dump dumps time in correct format

I'm not even sure what's causing this yet, but I think it's a good reason to clean this stuff up.

[BuonOmo]

If I comment out the jr_jackson specs completely and re-run rake, I see the following:

This is a bug in pure_json, I already opened an issue.

For the other ones, you just have to set the ENV variable locally (SKIP_ADAPTERS=jr_jackson,nsjsonserialization,json_pure)

So worst case scenario I would set this in the Rakefile, this would be a one LOC footprint rather than being all around the codebase :/

Have you seen the latest changes I made in the github-action PR ? Tests are basically passing, and I tried it locally as well :)

So if you really want a change, I'd advocate for this:

diff --git a/Rakefile b/Rakefile
index 9af21d7..dbb9ecd 100644
--- a/Rakefile
+++ b/Rakefile
@@ -16,6 +16,10 @@ namespace :adapters do
   end
 end
 
+# pure_json has a problem parsing time objects (see https://github.com/flori/json/issues/573)
+# jr_jackson and nsjsonserialization are outdated.
+ENV['SKIP_ADAPTERS'] = ENV.fetch('SKIP_ADAPTERS', 'json_pure,jr_jackson,nsjsonserialization')
+
 task :spec => %w[
   base_spec
   adapters:oj

Or updating the folder below and removing the skipping part in tests. The point still being avoiding spec mess where a user can never know what is running, and why. And centralize this behaviour as much as possible.

[djberg96]

@BuonOmo Nice catch on the pure_json bug, wonder how long that's been out there.

[sferik]

I’ll weigh in here:

1. I think it's important that new developers can clone this repo and get green tests locally by running the default rake task without having to set any environment variables.
2. I also think it's important that we don't hardcode too many conditions into the codebase and that there be one primary way to specify which adapters tests should run for.

I can think of a few solutions that would satisfy both of these conditions but I'm open to others. One option would be to make adapters opt-in instead of opt-out. Then we could set up a CI matrix where, for example, nsjsonserialization only runs on macOS. By default, we could run on a set of adapters that we'd expect to work on every platform. This could pave the way to adapters being released separately, as proposed in #201.

[djberg96]

@sferik I think you're wanting a proper spec_helper.rb with filters then.

BTW, does jrjackson only work with JRuby, or will it work with any Java-based implementation, e.g. TruffleRuby?

[djberg96]

Closing in favor of #215