feat(audits/server): Server response in failing audits #39
Conversation
enisdenjo
force-pushed
the
response-in-audit
branch
from
68cd6c0 to
205a30f
Compare
# [1.12.0](v1.11.0...v1.12.0) (2023-01-20) ### Bug Fixes * **audits/server:** Check the actual content encoding instead of the indication ([#41](#41)) ([67778a8](67778a8)) * **handler:** Response maker handles errors correctly ([#45](#45)) ([5a10e0b](5a10e0b)) ### Features * **audits/server:** Server response in failing audits ([#39](#39)) ([4385ecb](4385ecb))
|
🎉 This PR is included in version 1.12.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
| if (res.status !== code) { | ||
| throw new AuditError(res, `Response status code is not ${code}`); | ||
| } |
There was a problem hiding this comment.
We filter some of the SHOULD results from this test suite based on the text of the reason, so this broke our usage here:
https://github.com/apollographql/apollo-server/blob/37b3b7fb57ea105f40776166c9532536fd3f053d/packages/integration-testsuite/src/httpSpecTests.ts#L65
@enisdenjo This change also dropped the actual error code that was seen in the message (i.e. previously we expected a 400 in this string), it might be nice to restore res.status to the string here?
There was a problem hiding this comment.
Is there any expectation that the reason is stable (is this a breaking change that should be fixed in a patch?) or should we assume these might change and be prepared to update our filtering logic?
There was a problem hiding this comment.
Thanks for reporting! Sorry if this was a breaking change, those were not my intentions.
This change also dropped the actual error code that was seen in the message (i.e. previously we expected a 400 in this string), it might be nice to restore res.status to the string here?
Failing audits now have the complete response in the AuditFail.response object. Can this be helpful in your case?
Is there any expectation that the reason is stable (is this a breaking change that should be fixed in a patch?) or should we assume these might change and be prepared to update our filtering logic?
The reason itself is not subject to versioning, hence not a breaking change. However, I understand you use case - do you have any suggestions on identifying tests without depending on the reason? Maybe give each test an ID uniquely representing the case?
There was a problem hiding this comment.
@enisdenjo That's actually a great mitigation, thanks for the suggestion!
apollographql/apollo-server@31736b5
If the name is also not expected to be stable then a unique ID would be great. I would really like to be more explicit about which warnings we're allowing in our implementation. IDs would let me just do allowedWarningIDs.includes(audit.id) or something to that effect.
I suppose we could be doing this now with the names as I've alluded to, but maybe that's better as a human-readable description of the test which might be tweaked / improved upon in the future.
There was a problem hiding this comment.
@trevor-scheer I'd love to hear your thoughts on #49, would that be a fitting enhancement?
Closes #10