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.

Sign up for GitHub

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

Jump to bottom

Update CI to use Node 12 #2538

Merged
merged 4 commits into from
Mar 3, 2020
Merged

Update CI to use Node 12 #2538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b67cf38
Update CI to use Node 12
taneliang Mar 3, 2020
a4c698f
Upgrade mock-fs dep to fix one test failure
taneliang Mar 3, 2020
3deb720
Fix nus-api concurrency test
taneliang Mar 3, 2020
42492f2
Merge branch 'master' into node-12
taneliang Mar 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ version: 2

defaults: &defaults
docker:
- image: circleci/node:10
- image: circleci/node:12

restore_cache: &restore_node_deps
name: Restore node_modules cache
Expand Down
4 changes: 2 additions & 2 deletions scrapers/nus-v2/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"@types/jest": "23.3.13",
"@types/joi": "14.3.1",
"@types/lodash": "4.14.144",
"@types/mock-fs": "3.6.30",
"@types/mock-fs": "4.10.0",
"@types/oboe": "2.0.28",
"@types/promise-queue": "2.2.0",
"@types/ramda": "0.25.50",
Expand All @@ -38,7 +38,7 @@
"http-status": "1.3.1",
"jest": "24.9.0",
"jest-junit": "10.0.0",
"mock-fs": "4.8.0",
"mock-fs": "4.11.0",
"prettier": "1.19.1",
"swagger-ui-dist": "3.21.0",
"ts-jest": "24.2.0",
Expand Down
8 changes: 6 additions & 2 deletions scrapers/nus-v2/src/services/nus-api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ describe(callApi, () => {

describe(NusApi, () => {
test('should enforce maximum concurrency', async () => {
expect.assertions(6);
expect.assertions(7);

mockedAxios.post.mockResolvedValue(
mockResponse({ code: '00000', msg: '', data: 'Turn down for whaaaaat?' }),
Expand All @@ -109,19 +109,23 @@ describe(NusApi, () => {
const p3 = api.callApi('test-3', {});
const p4 = api.callApi('test-4', {});

// Expect 2 requests to have started, with 2 more waiting to be started.
expect(mockedAxios.post).toBeCalledTimes(2);
expect(api.queue.getPendingLength()).toEqual(2);
expect(api.queue.getQueueLength()).toEqual(2);

await p1;
await p2;

// Expect remaining 2 requests to have started.
expect(mockedAxios.post).toBeCalledTimes(4);
expect(api.queue.getPendingLength()).toEqual(0);
Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

I think you're right, I might have mistaken getPendingLength for the number of buffered tasks

Copy link
Member Author

Choose a reason for hiding this comment

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

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

expect(api.queue.getQueueLength()).toEqual(0);

await p3;
await p4;

// Expect no more pending requests.
expect(api.queue.getPendingLength()).toEqual(0);
expect(api.queue.getQueueLength()).toEqual(0);
});
});
Loading