Skip to content

New matcher that asserts presence of the given fields #35

Open
wants to merge 7 commits into from

4 participants

@AlexanderPavlenko

No description provided.

@laserlemon laserlemon commented on an outdated diff Oct 28, 2012
lib/json_spec/matchers.rb
@@ -25,6 +26,10 @@ def have_json_type(type)
def have_json_size(size)
JsonSpec::Matchers::HaveJsonSize.new(size)
end
+
+ def have_json_fields(size)
@laserlemon
Collective Idea member
laserlemon added a note Oct 28, 2012

This should be fields rather than size, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@laserlemon
Collective Idea member

Please reference #25 to see what I'd like to see in order to pull this in. Thank you!

@AlexanderPavlenko

@laserlemon check out latest commit

@inossidabile

@laserlemon are there any other problems with this pr? :)

@AlexanderPavlenko

@charlierudolph nice catch, fixed

@laserlemon
Collective Idea member

@AlexanderPavlenko Thanks for all of your work! There are just a couple things I would change and I'll comment on those individually in the diff. Thank you again!

@laserlemon laserlemon and 1 other commented on an outdated diff Feb 14, 2013
features/keys.feature
+ Given the JSON is:
+ """
+ {
+ "a": 1,
+ "b": 2,
+ "c": 3,
+ "z" : {
+ "d": 4,
+ "e": 5
+ }
+ }
+ """
+
+ Scenario: Base paths
+ When I get the JSON
+ Then the JSON should have keys "a, b, c"
@laserlemon
Collective Idea member
laserlemon added a note Feb 14, 2013

Since "a, b, c" is a valid JSON key, we should separate each key out into its own quotes:

Then the JSON should have keys "a", "b", "c"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@laserlemon laserlemon and 1 other commented on an outdated diff Feb 14, 2013
lib/json_spec/cucumber.rb
@@ -59,7 +59,7 @@
end
Then /^the (?:JSON|json)(?: response)?(?: at "(.*)")? should have the following:$/ do |base, table|
- table.rows.each do |path, value|
+ table.raw.each do |path, value|
@laserlemon
Collective Idea member
laserlemon added a note Feb 14, 2013

This doesn't belong in this pull request. I believe that is a valid fix, referencing #33, but it should be in its own PR with tests.

I found this hard to test nicely within current tests suite, because assertion which is true for N items is also true for any subset of those items.
Also, I faced (very accidently) this issue here — https://github.com/collectiveidea/json_spec/pull/35/files#L2R74 , so I don't see many sense in keeping that aside and reverting this line.

@laserlemon
Collective Idea member
laserlemon added a note Feb 14, 2013

We don't need to revert it if we can add a test. If he problem is that the first element is ignored, make the first element invalid and the rest valid. Then assert a non-match.

oh, good point

Wait, not so good. Two things:
1) There is no such step "the JSON should not have the following"
2) Is that really the expected behavior for mixed valid/invalid table entries? Logically, their set is neither included, nor excluded from the tested one. They just intersect. So it should fail.

@laserlemon
Collective Idea member
laserlemon added a note Feb 14, 2013
  1. Good point.
  2. I don't understand.

I think the expected behavior would be that if a single invalid entry is included in the table, the assertion should fail. That said, if we don't have a negative step definition, we should be testing that the positive assertion fails.

I mean this example should fail, but it pass because at least one entry is invalid.

And the JSON should not have the following keys:
  | invalid           |
  | valid             |
  | another_valid     |
@laserlemon
Collective Idea member
laserlemon added a note Feb 14, 2013

Right. So we could write a test to assert that that fails. Make sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@charlierudolph charlierudolph commented on the diff Nov 27, 2013
lib/json_spec/cucumber.rb
@@ -70,6 +70,16 @@
end
end
+Then /^the (?:JSON|json)(?: response)?(?: at "(.*)")? should( not)? have the following keys:$/ do |base, negative, table|
+ match = have_json_keys(table.raw)
+ match = match.at_path(base) if base

Please rename base to path to be consistent.
Can then shorten to one line as its works if path is nil.

match = have_json_keys(table.raw).at_path(path)

Nice drying up. Will make use of this pattern to dry up the rest of cucumber later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@charlierudolph charlierudolph commented on the diff Nov 27, 2013
README.md
@@ -11,6 +11,7 @@ json_spec defines five new RSpec matchers:
* `be_json_eql`
* `include_json`
* `have_json_path`
+* `have_json_keys`

Also need to update the count of methods this gem adds a few lines above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.