chore/Issue 1016: Replace the use of request with fetch in test cases #1172
Conversation
|
|
||
| done(); | ||
| it('should return a 200', function() { | ||
| expect(response.status).to.equal(200); |
There was a problem hiding this comment.
@thescientist13
Just wanted to double check on these changes before I start to apply them to the other files. I only really ask because it doesn't seem like anything broke when I changed the specs.
There was a problem hiding this comment.
yeah, this looks to be on the right track!
I feel like there was a case I tried that was tricky and couldn't get it work. Maybe do a "spot" test on the request.post usages in this spec on the release/0.29.0 branch?
Also worth testing in any serve based test case, I noticed in those cases I had to use .127.0.0.1 and maybe that was where I got stuck?
But if you can get that cross section of test cases working, then I think it should be full steam ahead at that point.
There was a problem hiding this comment.
Maybe do a "spot" test....
Was successful on the spot test: release/0.29.0...DevLab2425:greenwood:issue-1016-release-update
I had to use .127.0.0.1 .....
Will report back on the IP-based tests.
There was a problem hiding this comment.
I was also successful with replacing the IP with localhost.
Updated compare: release/0.29.0...DevLab2425:greenwood:issue-1016-release-update
There was a problem hiding this comment.
Do you want me to target the v0.29.0 branch for this PR too? It'll have ~90 updates/~25 files with all of the replacements.
There was a problem hiding this comment.
Yeah, if you've got it all figured out, feel free to target that release branch! 🙏
DevLab2425
force-pushed
the
issue-1016-replace-request-with-fetch
branch
from
31581fb
to
d351f55
Compare
DevLab2425
changed the title
DevLab2425
force-pushed
the
issue-1016-replace-request-with-fetch
branch
from
d351f55
to
bc5eb6c
Compare
DevLab2425
force-pushed
the
issue-1016-replace-request-with-fetch
branch
4 times, most recently
from
9864116
to
76569d9
Compare
|
Got a little time, let me see if I can help with the build failures and merge conflicts. |
.mocharc.cjs
Outdated
| @@ -1,3 +1,5 @@ | |||
| module.exports = { | |||
| timeout: 90000 | |||
| timeout: 90000, | |||
| bail: true, | |||
There was a problem hiding this comment.
could you expand a little bit on what these options introduce?
edit: looks like bail will kill the test run as soon as it hits an error? (as opposed to running all specs and providing a full report at the end?)
There was a problem hiding this comment.
I ended up rolling this one back since it was a little frustrating to have to keep re-running specs and then getting halted when one fails, re-starting the runner, bailing at the next failure, etc. It can be nice to let things keep running to see all the failures in totality during a single run.
Perhaps this could be useful as a CI only flag? So we don't keep wasting build time after a failure? 🤔
thescientist13
force-pushed
the
issue-1016-replace-request-with-fetch
branch
from
90bd9a0
to
4de60da
Compare
There was a problem hiding this comment.
Phew, thanks for getting this effort started @DevLab2425 ! After rebasing and resolving the merge conflicts, I was able to get to the rest of the test cases, so this is all looking good to go now!
Feels a lot more lightweight and intuitive using fetch 💯
thescientist13
changed the title
thescientist13
merged commit 7c9c23f
into
ProjectEvergreen:release/0.29.0
…#1172) * Issue 1016: Initital attempt at using fetch over request * Issue-1016: Refactor IP to use localhost url * Issue-1016: Initial POST refactor to fetch * Issue-1016: Progress on removal of request * Issue-1016: Additional removal of request * Issue-1016: Finalize plugin tests * use response.text() * refactor init spec to fetch * all specs refactored to use fetch * refactoring all specs --------- Co-authored-by: Owen Buckley <[email protected]>
Related Issue
#1016
Summary of Changes
requestwithfetchfetchresponse API