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

Generalise request matching in PublishingApi assertions #337

Merged
merged 1 commit into from Jul 30, 2015

Conversation

heathd
Copy link
Contributor

@heathd heathd commented Jul 30, 2015

The assertions provided by the PublishingApi test helpers are useful.
The existing implementation allows an assertion to require that a
request was made to a publishing api endpoint with a hash containing a
certain list of required attributes.

However the underlying matching process would only take into account the
outermost level of the hash when performing the comparison. This means
that you could make an flexible assertion about the required elements at
the top level of a document sent to the publishing api, but if you
wanted to make assertions about the details section or elements nested
within the details section, you could only perform an exact match on the
details hash.

This would lead to brittle, difficult to maintain tests.

This commit allows passing custom matcher predicates to the assertions
and defines a new matcher which can match nested structures more
loosely.

The original strict matching behaviour is retained as a default to avoid
possible unintended side-effects on the test suites of other apps.

We considered making the looser behaviour the default, but after
reviewing the usage of some other apps I had enough doubt about that I
thought it would be better to be conservative and avoid changing the
default matching behaviour.

To use the assertions with the looser matcher you can do the following:

assert_publishing_api_put_item(
  base_path,
  request_json_including(details: {title: "My title"})
)

Note that both matchers will convert symbols to strings so the
hash can use either symbol or string keys.

Feedback suggestions

  • Do the predicate names make sense? (request_json_matching, request_json_strictly_matching)
    • names changed
  • Do you agree with the conservative approach?

@heathd heathd force-pushed the generalise-publishing-api-assertion-matchers branch 2 times, most recently from 3d3b748 to 179eeb4 Compare July 30, 2015 14:38
when Hash
return false unless actual_value.is_a?(Hash)
expected_value.all? do |expected_sub_key, expected_sub_value|
key = symbols_as_strings(expected_sub_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just call expected_sub_key.to_s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

keys could in theory be integers or booleans

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just read the JSON spec, I was mistaken. Object keys must be strings.

That simplifies things.

@elliotcm
Copy link
Contributor

General approach seems fine. Specifics seem mostly fine.

Regarding your questions, I'd personally assume that request_json_matching performed an exact match rather than a partial match. Should we follow the RSpec format where they use hash_including?

I'm a little unsure about extending the partial matching to hashes within arrays. Do we have a concrete use case for that?

@heathd
Copy link
Contributor Author

heathd commented Jul 30, 2015

thanks for your comments

re: naming, how about

  • change request_json_strictly_matching to request_json_matching
  • change request_json_matching to request_json_including

re: use case for matching hashes within arrays, yes that's needed for matching the manuals table of contents which look like this:

    "details": {
      "child_section_groups": [
        {
          "title": "Contents",
          "child_sections": [
            {
              "title": "Child title",
              "description": "Child description",
              "base_path": "/guidance/manual/child-title",
            }
          ]
        }
      ]
    }

@elliotcm
Copy link
Contributor

Name change sounds good to me.

@heathd heathd force-pushed the generalise-publishing-api-assertion-matchers branch 2 times, most recently from 76e79d2 to b65fae4 Compare July 30, 2015 15:34
The assertions provided by the PublishingApi test helpers are useful.
The existing implementation allows an assertion to require that a
request was made to a publishing api endpoint with a hash containing a
certain list of required attributes.

However the underlying matching process would only take into account the
outermost level of the hash when performing the comparison. This means
that you could make an flexible assertion about the required elements at
the top level of a document sent to the publishing api, but if you
wanted to make assertions about the details section or elements nested
within the details section, you could only perform an exact match on the
details hash.

This would lead to brittle, difficult to maintain tests.

This commit allows passing custom matcher predicates to the assertions
and defines a new matcher which can match nested structures more
loosely.

The original strict matching behaviour is retained as a default to avoid
possible unintended side-effects on the test suites of other apps.

We considered making the looser behaviour the default, but after
reviewing the usage of some other apps I had enough doubt about that I
thought it would be better to be conservative and avoid changing the
default matching behaviour.

To use the assertions with the looser matcher you can do the following:

```
assert_publishing_api_put_item(
  base_path,
  request_json_including(details: {title: "My title"})
)
```

Note that both matchers will convert symbols to strings so the
hash can use either symbol or string keys.
@heathd heathd force-pushed the generalise-publishing-api-assertion-matchers branch from b65fae4 to a90d053 Compare July 30, 2015 15:38
elliotcm added a commit that referenced this pull request Jul 30, 2015
…tion-matchers

Generalise request matching in PublishingApi assertions
@elliotcm elliotcm merged commit 64530f9 into master Jul 30, 2015
@elliotcm elliotcm deleted the generalise-publishing-api-assertion-matchers branch July 30, 2015 15:40
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.

None yet

2 participants