Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Avoid Skip in test, have a unified test for order

  • Loading branch information...
commit 90c450f6cd792187eeb1e039e0ff3722beb8b5c1 1 parent 9f8116f
@gaurish gaurish authored
Showing with 7 additions and 10 deletions.
  1. +7 −10 activemodel/test/cases/serializers/json_serialization_test.rb
View
17 activemodel/test/cases/serializers/json_serialization_test.rb
@@ -155,21 +155,18 @@ def @contact.favorite_quote; "Constraints are liberating"; end
end
end
- test "as_json should keep the MRI default order in the hash" do
- skip "on JRuby as order is different" if defined? JRUBY_VERSION
+ test "as_json should keep the default order in the hash" do
json = @contact.as_json
- assert_equal %w(name age created_at awesome preferences), json.keys
- end
-
- test "as_json should keep the JRuby default order in the hash" do
- skip "on MRI as order is different" unless defined? JRUBY_VERSION
- json = @contact.as_json
+ attributes_order = %w(name age created_at awesome preferences)
+ #Order on JRUBY is different
+ if defined? JRUBY_VERSION
+ attributes_order = %w(age name created_at awesome preferences)
+ end
- assert_equal %w(age name created_at awesome preferences), json.keys
+ assert_equal attributes_order, json.keys
@spastorino Owner

isn't much better to do json.keys.sort ?

@gaurish
gaurish added a note

the test or in the implementation(ActiveModel::Serialization#serializable_hash)? If we do it in test, we are not actually checking any order. this test will always be passing because of sort call.

@guilleiguaran Owner

The order in Rubinius is same as JRuby?

@gaurish
gaurish added a note

Yes. atleast on current rbx-head, not sure if that would change in future.

@guilleiguaran Owner

IMO the full test doesn't makes any sense, there are reasons to have this hash in a specific order?

@gaurish
gaurish added a note

I did some digging. it seems that earlier hash was explicitly sorted using sort call but this was later changed in #5678. As a result this test was added in 3152ee8.

so question is, does it matter if your json is ordered or random. I would say Yes, some kind of order whether its based on interal tables of ruby engine or based on sort call is welcome. your thoughts?

@spastorino Owner

Yeah the test doesn't make any sense now

@gaurish
gaurish added a note

then, lets delete this test :scissors:

@guilleiguaran Owner

done already :smile:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
-
test "from_json should work without a root (class attribute)" do
json = @contact.to_json
result = Contact.new.from_json(json)
@spastorino

isn't much better to do json.keys.sort ?

@gaurish

the test or in the implementation(ActiveModel::Serialization#serializable_hash)? If we do it in test, we are not actually checking any order. this test will always be passing because of sort call.

@guilleiguaran

The order in Rubinius is same as JRuby?

@gaurish

Yes. atleast on current rbx-head, not sure if that would change in future.

@guilleiguaran

IMO the full test doesn't makes any sense, there are reasons to have this hash in a specific order?

@gaurish

I did some digging. it seems that earlier hash was explicitly sorted using sort call but this was later changed in #5678. As a result this test was added in 3152ee8.

so question is, does it matter if your json is ordered or random. I would say Yes, some kind of order whether its based on interal tables of ruby engine or based on sort call is welcome. your thoughts?

@spastorino

Yeah the test doesn't make any sense now

Please sign in to comment.
Something went wrong with that request. Please try again.