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

Fail fast in regression test if response is not success #2519

Merged

Conversation

Projects
None yet
2 participants
@floehopper
Copy link
Contributor

commented May 11, 2016

Description

When generating regression test artefacts for the first time, we should never store a response with a non-success status code - this would be a sign that we've chosen the wrong questions & responses.

When running regression tests on subsequent occasions, it's more useful to see an assertion failure specifically about the non-success response, rather than an assertion about the response body diff not matching.

I did consider avoiding the duplication by putting the assertion inside SmartAnswerTestHelper#save_output, but I wanted to use ActionDispatch::Assertions::ResponseAssertions#assert_response which checks for status codes in the range 200-299 and that would've been awkward to do outside the SmartAnswersRegressionTest class.

External changes

None. This only affects test code.

@chrisroos chrisroos self-assigned this May 13, 2016

@chrisroos

This comment has been minimized.

Copy link
Contributor

commented May 13, 2016

This looks good to me, although I'm interested in what prompted this change. Did you run into a problem where regression test artefacts were being stored for non-success responses?

@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2016

@chrisroos:

Thanks for reviewing.

I'm interested in what prompted this change. Did you run into a problem where regression test artefacts were being stored for non-success responses?

No, but I did see them being generated and it reminded me that I think some of them have been committed by accident previously.

@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2016

Rebasing against master and force-pushing in preparation for merging.

Fail fast in regression test if response is not success
When generating regression test artefacts for the first time, we should never
store a response with a non-success status code - this would be a sign that
we've chosen the wrong questions & responses.

When running regression tests on subsequent occasions, it's more useful to see
an assertion failure specifically about the non-success response, rather than an
assertion about the response body diff not matching.

I did consider avoiding the duplication by putting the assertion inside
`SmartAnswerTestHelper#save_output`, but I wanted to use `ActionDispatch::Assertions::ResponseAssertions#assert_response` [1] which
checks for status codes in the range 200-299 and that would've been awkward to
do outside the `SmartAnswersRegressionTest` class.

[1]: http://api.rubyonrails.org/classes/ActionDispatch/Assertions/ResponseAssertions.html#method-i-assert_response

@floehopper floehopper force-pushed the fail-fast-in-regression-test-if-response-is-not-success branch from 40502a9 to f0b9c35 May 13, 2016

@floehopper floehopper merged commit ca870bb into master May 13, 2016

1 check passed

default "Build #5114 succeeded on Jenkins"
Details

@floehopper floehopper deleted the fail-fast-in-regression-test-if-response-is-not-success branch May 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.