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.

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

merged 3 commits into from
Feb 3, 2025

Conversation

drobnikj
Copy link
Member

@drobnikj drobnikj commented Jan 27, 2025

Fix flaky tests by changing the order of test executions 😄

@drobnikj drobnikj requested a review from B4nan January 27, 2025 12:55
@drobnikj drobnikj added the adhoc Ad-hoc unplanned task added during the sprint. label Jan 27, 2025
@drobnikj drobnikj marked this pull request as draft January 27, 2025 13:22
@drobnikj drobnikj removed the request for review from B4nan January 27, 2025 13:22
@drobnikj
Copy link
Member Author

Looks it generates other problem 🤯 let me check it.

@@ -492,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

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.

@B4nan B4nan changed the title fix(test): fix flaky test test: fix flaky test Jan 27, 2025
@B4nan
Copy link
Member

B4nan commented Jan 27, 2025

btw the fix commit type is for actual fixes of the library code (and those will end up in the generated changelog), fixing a flaky test has no effect on our users.

@B4nan
Copy link
Member

B4nan commented Jan 29, 2025

shall we merge this or why did you keep it as a draft?

@drobnikj drobnikj marked this pull request as ready for review February 3, 2025 12:01
@drobnikj
Copy link
Member Author

drobnikj commented Feb 3, 2025

let's merge this, seems like a good enough fix.

@drobnikj drobnikj requested a review from B4nan February 3, 2025 12:02
@github-actions github-actions bot added this to the 108th sprint - Platform team milestone Feb 3, 2025
@github-actions github-actions bot added t-platform Issues with this label are in the ownership of the platform team. tested Temporary label used only programatically for some analytics. labels Feb 3, 2025
@B4nan B4nan merged commit 393c9bf into master Feb 3, 2025
7 checks passed
@B4nan B4nan deleted the fix/remove-fleaky-test branch February 3, 2025 12:03
@fnesveda fnesveda added the validated Issues that are resolved and their solutions fulfill the acceptance criteria. label Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adhoc Ad-hoc unplanned task added during the sprint. t-platform Issues with this label are in the ownership of the platform team. tested Temporary label used only programatically for some analytics. validated Issues that are resolved and their solutions fulfill the acceptance criteria.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants