Skip to content

Conversation

@alumni
Copy link
Collaborator

@alumni alumni commented Nov 28, 2025

Description of change

Replace custom sleep() function with built-in scheduler.wait from node:timers/promises.

Modify wait times so the tests are less flaky.

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • This pull request links relevant issues as Fixes #00000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change

@qodo-free-for-open-source-projects

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Timeout Configuration

The timeout values have been changed from 150ms/0.2s to 200ms/0.3s. This change should be validated to ensure it doesn't make the timeout tests less effective at catching actual timeout issues, while still preventing flakiness.

const timeoutMs = 200
const longQueryTimeSec = 0.3
const shortQueryTimeSec = 0.005
Assertion Change

The assertion was changed from checking inequality (not.eql) to checking greater-than relationship. While this is more precise, verify that this change is intentional and that the 100ms wait is sufficient for all database systems to register a timestamp difference.

expect(
    loadedPostAfterUpdate.updatedAt.getTime(),
).to.be.greaterThan(loadedPostBeforeUpdate.updatedAt.getTime())
Cache Timing

Multiple cache expiration tests now wait 1010ms instead of 1000ms. While this adds a safety margin, verify that the cache expiration is set to 1000ms in the test setup, and confirm that 1010ms is sufficient across all test environments without being excessive.

await scheduler.wait(1010)

@qodo-free-for-open-source-projects
Copy link

qodo-free-for-open-source-projects bot commented Nov 28, 2025

PR Code Suggestions ✨

Latest suggestions up to 19258f9

CategorySuggestion                                                                                                                                    Impact
General
Increase wait time for timestamp

Increase the scheduler wait time from 100ms to 1010ms to ensure reliable
timestamp differentiation in database operations and prevent flaky tests.

test/functional/repository/basic-methods/repository-basic-methods.ts [470]

-await scheduler.wait(100)
+await scheduler.wait(1010)
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that a 100ms wait might be too short for reliably detecting timestamp changes in a database, potentially causing flaky tests. Increasing the wait time improves test robustness.

Medium
  • More

Previous suggestions

Suggestions up to commit 3e31162
CategorySuggestion                                                                                                                                    Impact
General
Increase wait time for timestamp

Increase the scheduler.wait time from 100ms to 1010ms to prevent flaky tests
related to timestamp precision in different database systems.

test/functional/repository/basic-methods/repository-basic-methods.ts [470]

-await scheduler.wait(100)
+await scheduler.wait(1010)
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies a potential for flaky tests due to a short wait time, and increasing it aligns with other tests in the PR, improving test reliability.

Low
Suggestions up to commit 920d26e
CategorySuggestion                                                                                                                                    Impact
Possible issue
Increase cache expiration wait time

Increase the wait time from 1010ms to 1100ms for cache expiration tests. This
provides a larger buffer over the 1000ms cache duration, making the tests more
reliable and less prone to flakiness.

test/functional/cache/custom-cache-provider.ts [103-104]

 // give some time for cache to expire
-await scheduler.wait(1010)
+await scheduler.wait(1100)
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a potential source of test flakiness by pointing out that a 10ms buffer for a 1000ms cache timeout is too small, and proposes a more robust value.

Low
Suggestions up to commit f48412b
CategorySuggestion                                                                                                                                    Impact
Possible issue
Increase wait time to prevent flaky tests

To prevent flaky tests due to low database timestamp precision, increase the
wait time from 100ms to over a second (e.g., 1001ms) before updating the entity.

test/core/column-kinds/update-date-column/update-date-column.ts [111-125]

-// wait a bit
-await scheduler.wait(100)
+// wait a bit to ensure updated date will be different
+await scheduler.wait(1001)
 
 // update post once again
 post.title = "Updated Title"
 await postRepository.save(post)
 
 // check if date was updated
 const loadedPostAfterUpdate =
     await postRepository.findOneByOrFail({
         id: post.id,
     })
 expect(
     loadedPostAfterUpdate.updatedAt.getTime(),
 ).to.be.greaterThan(loadedPostBeforeUpdate.updatedAt.getTime())
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that a 100ms wait is insufficient to guarantee a change in the updatedAt timestamp on databases with low time precision, which could lead to flaky tests.

Medium

Copy link
Collaborator

@gioboa gioboa left a comment

Choose a reason for hiding this comment

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

👏LGTM

Copy link
Collaborator

@G0maa G0maa left a comment

Choose a reason for hiding this comment

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

🚀

@alumni alumni merged commit 835647a into typeorm:master Nov 29, 2025
61 checks passed
@coveralls
Copy link

Coverage Status

coverage: 80.768%. first build
when pulling 19258f9 on alumni:test/wait-time
into 5461927 on typeorm:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants