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

e2e tests: additional checks for application/json response headers #2609

Merged
merged 4 commits into from
Apr 15, 2021

Conversation

piotr-iohk
Copy link
Contributor

Issue Number

ADP-853

Overview

  • I have added additional checks in e2e tests checking that responses return 'content-type' => ['application/json;charset=utf-8'] header

Comments

Only 204 reponses do not return 'content-type' => ['application/json;charset=utf-8'] header. I assume that it is OK, since 204 returns no content?

I've temporarily cherry-picked c832eb5 from #2608, to be able to run all tests.

@piotr-iohk piotr-iohk added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label Apr 9, 2021
@piotr-iohk piotr-iohk self-assigned this Apr 9, 2021
@piotr-iohk piotr-iohk changed the title Piotr/additional checks adp 853 e2e tests: additional checks for application/json response headers Apr 9, 2021
@piotr-iohk piotr-iohk requested a review from rvl April 9, 2021 12:09
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Could we refactor the response assertions to keep it maintainable?
Almost every request will have application/json content-type, except for a handful.

@piotr-iohk
Copy link
Contributor Author

Could we refactor the response assertions to keep it maintainable?

@rvl you mean something like this -> 9e65a6c

@rvl
Copy link
Contributor

rvl commented Apr 14, 2021

Yes - a little. But it would be better if the response content type were checked automatically, so there doesn't need to be this assertion in every single test case. It could be tested when parsing the response (i.e. only decode as json if that's the content-type), or as part of the http status code check.

@piotr-iohk
Copy link
Contributor Author

Ok, gotcha. Currently it is like this:

expect(response).to have_http 201
expect(response).to have_expected_headers

which I could change to something like:

expect(response).to be_correct 201

where I could check both assertions, with additional logic that when response code is 204 the application/json header is not expected.

I guess the pros is that it is more concise, cons that two assertions combined in one... which is sort of anti-pattern I suppose. 🤔

@piotr-iohk piotr-iohk force-pushed the piotr/additional-checks-adp-853 branch from 0b81607 to 27df4ce Compare April 14, 2021 15:41
@piotr-iohk
Copy link
Contributor Author

Yes - a little. But it would be better if the response content type were checked automatically, so there doesn't need to be this assertion in every single test case.

Done in 27df4ce.

Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

👍

@rvl rvl force-pushed the piotr/additional-checks-adp-853 branch from 27df4ce to 1373e14 Compare April 15, 2021 05:23
@rvl
Copy link
Contributor

rvl commented Apr 15, 2021

I don't think it's an anti-pattern. I think it's better to explicitly test more high level things.

@piotr-iohk
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 15, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 2dcd243 into master Apr 15, 2021
@iohk-bors iohk-bors bot deleted the piotr/additional-checks-adp-853 branch April 15, 2021 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants