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

test: fix flaky test #630

Merged
merged 3 commits into from
Feb 3, 2025
Merged

test: fix flaky test #630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ca7194
Revert "test: Add debug log, to see why test randomly failed (#628)"
drobnikj Jan 27, 2025
08d149a
fix(test): fix flaky test
drobnikj Jan 27, 2025
ac5eeea
fix(test): fix flaky test
drobnikj Jan 27, 2025
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
12 changes: 1 addition & 11 deletions test/_helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,7 @@ const validateRequest = (query = {}, params = {}, body = {}, additionalHeaders =
};
const request = mockServer.getLastRequest();
const expectedQuery = getExpectedQuery(query);
if (query !== false) {
try {
expect(request.query).toEqual(expectedQuery);
} catch (err) {
// Debug log to check what was actually in request.
// It is happening that the "_batchAddRequests() works" randomly fails on CI, it can be caused by tests concurrency.
// If so we need to update how mockServer.getLastRequest() works.
console.log('Request:', request.route.path, request.route.methods, request.query);
throw err;
}
}
if (query !== false) expect(request.query).toEqual(expectedQuery);
if (params !== false) expect(request.params).toEqual(params);
if (body !== false) expect(request.body).toEqual(body);
Object.entries(headers).forEach(([key, value]) => {
Expand Down
60 changes: 31 additions & 29 deletions test/request_queues.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,35 +331,7 @@ describe('Request Queue methods', () => {
);
});

test('batchAddRequests() chunks large payload', async () => {
const queueId = 'some-id';
const requestsLength = 30;
const longString = 'a'.repeat(940_000);
const requests = new Array(requestsLength).fill(0).map((_, i) => ({ url: `http://example.com/${i}`, userData: { longString } }));

await client.requestQueue(queueId).batchAddRequests(requests);
// Based on size of one request and limit 9MB as max payload size only 10 requests should be sent in one batch
const firedRequests = mockServer.getLastRequests(requestsLength / 10);
const processedRequestUrls = [];
firedRequests.map((req) => {
expect(req.url).toEqual(`/${queueId}/requests/batch`);
req.body.forEach(({ url }) => {
processedRequestUrls.push(url);
});
});
expect(processedRequestUrls.length).toEqual(requestsLength);
expect(processedRequestUrls).toEqual(
expect.arrayContaining(requests.map((req) => req.url)),
);

// It throws error when single request is too big
const bigRequest = { url: `http://example.com/x`, userData: { longString: 'a'.repeat(9_500_000) } };
const requestsWithBigRequest = [...requests, bigRequest];
await expect(client.requestQueue(queueId).batchAddRequests(requestsWithBigRequest))
.rejects.toThrow(`RequestQueueClient.batchAddRequests: The size of the request with index: ${requestsWithBigRequest.length - 1}`);
});

test('_batchAddRequests() works', async () => {
test('batchAddRequests() propagates forefront', async () => {
const queueId = 'some-id';
const options = { forefront: true };
const requests = new Array(10).fill(0).map((_, i) => ({ url: `http://example.com/${i}` }));
Expand Down Expand Up @@ -491,5 +463,35 @@ describe('Request Queue methods', () => {
expect(browserRes.items).toEqual(browserRequests);
validateRequest({ limit: maxPageLimit }, { queueId });
});

// NOTE: This test needs to be last otherwise it will break other tests
Copy link
Member

Choose a reason for hiding this comment

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

it would be much better to remove the "dependency", tests should be ideally isolated. isn't it enough to use a different queue ID?

Copy link
Member Author

@drobnikj drobnikj Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@B4nan The problem is how the mock server is written
Checking request from mock server is done very basic using pop() method from list, see
https://github.com/apify/apify-client-js/blob/master/test/mock_server/server.js#L48
so changing ID does not help.

test('batchAddRequests() chunks large payload', async () => {
const queueId = 'some-id';
const requestsLength = 30;
const longString = 'a'.repeat(940_000);
const requests = new Array(requestsLength).fill(0).map((_, i) => ({ url: `http://example.com/${i}`, userData: { longString } }));

await client.requestQueue(queueId).batchAddRequests(requests);
// Based on size of one request and limit 9MB as max payload size only 10 requests should be sent in one batch
const firedRequests = mockServer.getLastRequests(requestsLength / 10);
const processedRequestUrls = [];
firedRequests.map((req) => {
expect(req.url).toEqual(`/${queueId}/requests/batch`);
req.body.forEach(({ url }) => {
processedRequestUrls.push(url);
});
});
expect(processedRequestUrls.length).toEqual(requestsLength);
expect(processedRequestUrls).toEqual(
expect.arrayContaining(requests.map((req) => req.url)),
);

// It throws error when single request is too big
const bigRequest = { url: `http://example.com/x`, userData: { longString: 'b'.repeat(9_500_000) } };
const requestsWithBigRequest = [...requests, bigRequest];
await expect(client.requestQueue(queueId).batchAddRequests(requestsWithBigRequest))
.rejects.toThrow(`RequestQueueClient.batchAddRequests: The size of the request with index: ${requestsWithBigRequest.length - 1}`);
validateRequest({}, { queueId }, false);
});
});
});
Loading