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

Added matchers for Hashes and JSON (which uses the Hash matcher) #79

Closed
wants to merge 38 commits into from

Conversation

playupchris
Copy link

The image belows shows an example diff, where:

  • red = items that were expected but were missing
  • magenta = items that were additional and unexpected
  • green = items that matched against a regular expression (eg.timestamps, or urls with ids in them)
  • blue = items that match a class (eg. "flames".is_a? String)
  • cyan = items that match a proc (eg. lambda { |x| %w(SHA FLA).include? x }.call("FLA") )

screenshot

@dchelimsky
Copy link
Contributor

We definitely need a better diff for matching hashes, but this relies on a terminal with color, which happens a lot, but far from all the time (think CI servers, etc). So we'd need a diff that doesn't rely on color either instead of or in addition to the color diff.

There are some more specific issues that I'll comment on directly in the diff.

end

failure_message_for_should do
ENV['GET_FAILURE_MESSAGE'] ? @difference.to_s.inspect: @difference.to_s
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this ENV var? I'd prefer to get rid of it.

Copy link
Author

Choose a reason for hiding this comment

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

It's not needed and we can get rid of it.
It was there to make it easier to create the failure messages for test cases.
I'll take it out and just toggle that on and off using a patch I could apply and un-apply when we need to make a new test.

@playupchris
Copy link
Author

As for, "need a diff that doesn't rely on color":
How about:

{
  "x" => {
    "a" => "ABC",
    "c" => ~ "CBC",
    "b" => - /[A-Z]{3}/+ "bbc"
  }
}

Where:

  • ~ "CBC" = a regex match (previously yellow)
  • - /[A-Z]{3}/ = a missing item (previously red)
  • + "bbc" = an extra item (previously green)

@playupchris
Copy link
Author

Hi again David,
I've implemented most of your suggestions.
What do you think now?

@playupchris
Copy link
Author

ping

@dchelimsky
Copy link
Contributor

@playupchris - I generally like this and want to include it, but I haven't had much spare time lately and this is the largest pull request I've encountered in a long while.

Just perusing the diff again, I think there are too many options here and users would find them confusing. I think we should only have four:

string.should be_json_matching(....) # partial match all
string.should be_json_matching_exactly(....) # exact match all

hash.should be_hash_matching(....) # partial match all
hash.should be_hash_matching_exactly(....) # exact match all

I'm also unsure as to whether we should include the operator matcher.

WDYT?

@myronmarston, @justinko - you guys want to weigh in on this?

@playupchris
Copy link
Author

@dchelimsky thanks for taking the time to look at this again.

I initially thought the operator matcher was useful as one already exists for arrays (although our matcher also matches arrays), but in the array matcher:

[1,2,3].should   =~ [2,3,1]   # => would pass

Whereas in our differ that same test would fail.

So I'm thinking now that maybe the operator is confusing, as well as not being backwardly compatible (for arrays).

I think (we both agree that) the json_matchers should mirror the hash_matcher (or not exist at all; and we just expect users to convert json to hash before doing the match).

As for the other matchers, I guess they are a little confusing.
To give a brief explanation:

be_hash_matching : expected and actual has the same keys and has matching values
be_hash_all_matching : as above, but the diff shows all matching values, not just the differences
be_hash_partially_matching : expected has a subset of actual's keys and has matching values
be_hash_all_partially_matching : as above, but the diff shows all matching values, not just the differences

I guess we could drop the alls, but they're kinda handy when you want to see exactly what was matched. On the other hand we could drop the non-alls, but they're also handy when you want to cut out all the noise from the diffs. In practice, the way we've been using it is to use the all or non-all matcher depending on a DIFF_SHOW_ALL environment variable. And we toggle the env variable depending on how closely we need to look at the error messages to be able to fix our code. So I'd be more than happy to drop the alls if we could get a more detailed error message by setting an env variable.

@playupchris
Copy link
Author

Here are some examples of the 4 matchers...

pat_datetime = /\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z/
expected = {
  "href"=>Regexp.compile("#{Regexp.quote '/api/goals/games/'}[0-9]+#{Regexp.quote '/matches/'}[0-9]+$"),
  "scheduled_start"=>pat_datetime,
  "networks"=>%w(abc nbc cnn),
  "expected_key"=>"expected_value",
  "end_date"=>pat_datetime,
  "home_team"=>{
    "name"=>String,
    "short_name"=>lambda { |x| x.is_a?(String) and x.size == 3 },
    "link"=>[{"href"=>"http://puge.example.org/api/goals/teams/FLA/players", "rel"=>"players"}],
    "href"=>"http://puge.example.org/api/goals/teams/FLA"
  },
  "away_team"=>{
    "name"=>"sharks",
    "short_name"=>"SHA",
    "link"=>[{"href"=>"http://puge.example.org/api/goals/teams/SHA/players", "rel"=>"players"}],
    "href"=>"http://puge.example.org/api/goals/teams/SHA"
  }
}
actual = {
  "href"=>"http://puge.example.org/api/goals/games/635/matches/832",
  "scheduled_start"=>"2010-01-01T00:00:00Z",
  "networks"=>["abc", "cnn", "yyy", "zzz"],
  "unexpected_key"=>"unexpected_value",
  "end_date"=>"2010-01-01T01:00:00Z",
  "home_team"=>{
    "name"=>"flames",
    "short_name"=>"FLA",
    "link"=>[{"href"=>"http://puge.example.org/api/goals/teams/FLA/players", "rel"=>"players"}],
    "href"=>"http://puge.example.org/api/goals/teams/FLA"
  },
  "away_team"=>{
    "name"=>"sharks",
    "short_name"=>"unexpected2",
    "link"=>[{"href"=>"http://puge.example.org/api/goals/teams/SHA/players", "rel"=>"players"}],
    "href"=>"http://puge.example.org/api/goals/teams/SHA"
  }
}

be_hash_matching : expected and actual has the same keys and has matching values

> RSpec::Matchers::Differ::Difference.new(expected, actual).details
{
  "networks"=>[
    - "nbc"+ "cnn",
    - "cnn"+ "yyy",
  + "zzz"
  ],
  "away_team"=>{
    "short_name"=>- "SHA"+ "unexpected2"
  },
- "expected_key"=>"expected_value",
+ "unexpected_key"=>"unexpected_value"
}
Where, - 4 missing, + 5 additional

be_hash_all_matching : as above, but the diff shows all matching values, not just the differences

> RSpec::Matchers::Differ::Difference.new(expected, actual, :show_all=>true).details`
{
  "href"=>~ http://puge.example.org(/api/goals/games/635/matches/832),
  "scheduled_start"=>~ (2010-01-01T00:00:00Z),
  "networks"=>[
    - "nbc"+ "cnn",
    - "cnn"+ "yyy",
  + "zzz"
  ],
  "end_date"=>~ (2010-01-01T01:00:00Z),
  "home_team"=>{
    "name"=>: flames,
    "short_name"=>{ FLA
  },
  "away_team"=>{
    "short_name"=>- "SHA"+ "unexpected2"
  },
- "expected_key"=>"expected_value",
+ "unexpected_key"=>"unexpected_value"
}
Where, - 4 missing, + 5 additional, ~ 3 match_regexp, : 1 match_class, { 1 match_proc

be_hash_partially_matching : expected has a subset of actual's keys and has matching values

RSpec::Matchers::Differ::PartialDifference.new(expected, actual).details
{
  "networks"=>[
    - "nbc"+ "cnn",
    - "cnn"+ "yyy"
  ],
  "away_team"=>{
    "short_name"=>- "SHA"+ "unexpected2"
  },
- "expected_key"=>"expected_value"
}
Where, - 4 missing, + 3 additional

be_hash_all_partially_matching : as above, but the diff shows all matching values, not just the differences

RSpec::Matchers::Differ::PartialDifference.new(expected, actual, :show_all=>true).details
{
  "href"=>~ http://puge.example.org(/api/goals/games/635/matches/832),
  "scheduled_start"=>~ (2010-01-01T00:00:00Z),
  "networks"=>[
    - "nbc"+ "cnn",
    - "cnn"+ "yyy",
  + "zzz"
  ],
  "end_date"=>~ (2010-01-01T01:00:00Z),
  "home_team"=>{
    "name"=>: flames,
    "short_name"=>{ FLA
  },
  "away_team"=>{
    "short_name"=>- "SHA"+ "unexpected2"
  },
- "expected_key"=>"expected_value",
+ "unexpected_key"=>"unexpected_value"
}
Where, - 4 missing, + 5 additional, ~ 3 match_regexp, : 1 match_class, { 1 match_proc

(NB. "matched values" means eg. foo matches String. or "foo" matches /[a-z]{3}/ and doesn't include equivalent values eg. "foo" matches "foo".)

@dchelimsky
Copy link
Contributor

So whether "all" is part of the name effects the output, but not the success/failure of the matcher. I think the env var is one good option. Another would be to add an additional argument to the matcher:

actual.should be_hash_including(expected, :show_all => true)

Or perhaps go the other way, so the norm is the show all, but for larger structures you can constrain it:

actual.should be_hash_including(expected, :condense_output => true)

I don't like :condense_output specifically, but you get the idea.

Thoughts?

@playupchris
Copy link
Author

I like that.

We'd end up setting the constraining argument with our own env variable, but doing it this way is more flexible.

How bout :verbose => false as the default?
(Or :quiet => true ... but personally I prefer :verbose => false)

@playupchris
Copy link
Author

Requests have been made to swap the green and yellow colours (because "green" currently means "additional" items, whereas people are used to "green" meaning "good").

@playupchris
Copy link
Author

I've split the Difference class off into it's own gem diff_matcher and changed the be_json_matching and be_hash_matching matchers to take the optional arguments as previously discussed.

@playupchris
Copy link
Author

  • changed over matchers from dsl to class
  • renamed be_hash_matching to be_matching as it actually matches more than just hashes.
  • removed :verbose option from diff_matcher and made it the default (:quiet still exists)

@dchelimsky
Copy link
Contributor

Hey @playupchris - thanks for all your hard work on this, and for extracting the diff out to a separate gem. Apologies if you did that due to this taking so long, but I think it's actually a good thing in the end. Once I saw all the differs you mention in the README, I thought a better direction for rspec-expectations would be to add better support for choosing your own differ. We had that in rspec-1, and I didn't include it in rspec-2 (which was a complete rewrite), but I think it would work well now. Rather than merging this pull, I'm going to add support for choosing your own differ to the next release (2.9 - 2.8 is already out as a release candidate and I don't want this to hold that up from going final).

I've created a new issue (#97) for this, and welcome your input on that issue.

Cheers,
David

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.

2 participants