Update CI to use Node 12 #2538
Update CI to use Node 12 #2538
Conversation
Our Docker setup already uses Node 12.
Codecov Report
@@ Coverage Diff @@
## master #2538 +/- ##
=======================================
Coverage 45.52% 45.52%
=======================================
Files 244 244
Lines 5162 5162
Branches 1181 1181
=======================================
Hits 2350 2350
Misses 2812 2812 Continue to review full report at Codecov.
|
mtimeMs was returing nanoseconds instead of milliseconds. See: tschaub/mock-fs#276
| expect(mockedAxios.post).toBeCalledTimes(4); | ||
| expect(api.queue.getPendingLength()).toEqual(0); |
There was a problem hiding this comment.
@ZhangYiJiang any reason this should be 0? getPendingLength returns 2 in Node 12 but returns 0 in Node 10.
I think a pending length of 2 may make sense since p3 and p4 may have already been dequeued (and thus started) after the values are resolved. The relevant library code is at https://github.com/promise-queue/promise-queue/blob/fd3ea37d56def993da2b34dc1a87f66e92b9e808/lib/index.js#L153-L160.
Either way, I think the remaining 2 checks on lines 121-122 should be enough to ensure that all the requests fire at some point. Lines 113-115 now ensure that we hit the concurrency limit without exceeding it.
There was a problem hiding this comment.
I think you're right, I might have mistaken getPendingLength for the number of buffered tasks
There was a problem hiding this comment.
Understandable, both getPendingLength and getQueueLength have extremely confusing names and comments. I had to read the source to figure out what they actually referred to
|
Deployment preview for |
Our Docker setup already uses Node 12 LTS. Once this PR is merged, I'll upgrade Node on the 1. server from Node 10 to Node 12.