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

Check forbidden cacheability #6

Merged

Conversation

patrobinson
Copy link
Contributor

There is a good use case for checking a page is both forbidden and not cacheable. (Steam could do with something like this :P)

We shouldn't be checking the response code inside the not_be_cached matcher, instead we should be chaining matchers together if required. This PR introduces this functionality, so you are able to do:

expect(url).to not_be_cached.and be_forbidden

Patrick Robinson added 4 commits January 14, 2016 15:11
Let users explicitly check the return code
So we can check for an unsuccessful response
@patrobinson
Copy link
Contributor Author

I have a spec failure:

Failures:

  1) be_served_from_origin should fail on 300 and correct origin
     Failure/Error: expect { expect(DOMAIN + '/redirect').to be_served_from_origin('originsite.example.com') }
       expected RuntimeError but nothing was raised
     # ./spec/functional/x_cache_headers_spec.rb:47:in `block (2 levels) in <top (required)>'

Finished in 1.03 seconds (files took 0.48039 seconds to load)
66 examples, 1 failure, 2 pending

But digging into this failure I believe it's not caused by my changes, simple unmasking a failure that was somehow masked. This is probably because your matchers are returning RuntimeErrors rather than false (see #5), but I can't figure out where this issue lies.

Also the x_cache_headers matchers look weird, I'm not sure if it's appropriate to call an expectation within a matcher? I would instead expect all functionality to be split out into methods, as this is both confusing and also possibly a cause of this failure.

Patrick Robinson added 10 commits January 15, 2016 11:13
otherwise continue with further checks. Rather than assuming success if
this check succeeds, which causes a failure of 'should fail on 300 and
correct origin' test.
The error claims a match is not found, but the match is not done in this
method, it's within the be_served_from_origin matcher. It fails if the
header exists? Any successful tests were masked until the previous commit
Wow that syntax was confusing
This is how a matcher should behave
Let RSpec raise an exception
This just makes this code more confusing...
And ensure the response code is successful
Simplifying and removing abstractions reduces code count and complexity
significantly.
@patrobinson
Copy link
Contributor Author

Ok, I've significantly refactored the x_cache_headers matchers, to make it more readable and now the tests pass ✨

@beegibson
Copy link
Collaborator

Thanks for the PR, I'll review it at some point over this weekend.

@beegibson
Copy link
Collaborator

Thanks, the x_cache stuff is so much clearer now!

beegibson added a commit that referenced this pull request Jan 18, 2016
@beegibson beegibson merged commit 9aa5290 into realestate-com-au:master Jan 18, 2016
@patrobinson patrobinson deleted the check-forbidden-cacheability branch January 18, 2016 01:39
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