replace old ocsp and crl apiTests with new integration tests#120
replace old ocsp and crl apiTests with new integration tests#120
Conversation
Ethan-Arrowood
left a comment
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
This is looking great. Love to see some extensive use of the new integration testing framework.
I'm most concerned with two things:
- configurable ports (can introduce major flakiness)
- Assertions seem to be okay with
200,400, and other status codes. Unless it really is random or undetermined, these scenarios feel like specific cases we should be asserting for. What would cause a request to sometimes 200 and other times 400 when it comes to certificate verification? Seems suspicious to me, but maybe there is something to all of that I'm not aware of or considering.
Finally, is there any additional documentation for the new utils (or tests) that needs to be added to the integration testing docs content?
|
@Ethan-Arrowood thanks for all the feedback! I'll get back to this soon - head's down on something else. RE response codes I've cared more about 'access denied' vs 'okay-ish'. I have no problem refining these tests though to catch any potential false passes. |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
I haven't looked at the details and trust you are sorting that out, but I certainly approve at a high-level.
heskew
force-pushed
the
cert-verification-integration-tests
branch
7 times, most recently
from
f23bf1a to
5f3dc41
Compare
|
Just took a read through your replies. Sounds like you're on a much better track now. I'll wait until you mark this as ready for review before going through the code again. |
heskew
force-pushed
the
cert-verification-integration-tests
branch
from
3efea32 to
9ce6ab2
Compare
|
@Ethan-Arrowood - please give this another look whenever you get a chance |
|
Sorry for the delay; can you fix the conflicts before I review? |
heskew
force-pushed
the
cert-verification-integration-tests
branch
2 times, most recently
from
1b5fb69 to
a10e938
Compare
…ertain reliability
heskew
force-pushed
the
cert-verification-integration-tests
branch
2 times, most recently
from
b01e519 to
1747c1c
Compare
heskew
force-pushed
the
cert-verification-integration-tests
branch
from
1747c1c to
1f04671
Compare
@Ethan-Arrowood no worries and you've been super busy with other things. rebased and addressed an edge case that surfaced during testing. |
Ethan-Arrowood
left a comment
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
This is looking good. The only thing is it seems like you have a bad merge here. This is adding back things removed from harperLifecycle.ts previously. Fix that and then I think you're good to go.
| ); | ||
| const dataRootDir = await mkdtemp(dataRootDirPrefix); | ||
| await cp(fixturePath, join(dataRootDir, 'components', basename(fixturePath)), { recursive: true }); | ||
| (ctx as any).harper = { dataRootDir }; |
There was a problem hiding this comment.
| (ctx as any).harper = { dataRootDir }; | |
| ctx.harper = { dataRootDir }; |
This should be fine?
There was a problem hiding this comment.
Type '{ dataRootDir: string; }' is missing the following properties from type 'HarperContext': admin, httpURL, operationsAPIURL, hostname, process
4 participants
Resolves #118 and #119