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

feat(pool): added acquireTimeout supports for pool #1919

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

fenying
Copy link

@fenying fenying commented Mar 27, 2023

This PR added implements for PoolConfig.acquireTimeout supports, as issue #673 mentioned.

  • The default value is 10000ms, as described in the comment in TypeScript definitions
  • It could be disabled by setting to 0.
  • Throws an error if acquire timeout.

@fenying
Copy link
Author

fenying commented Mar 27, 2023

P.S. I am not sure if it's corrected writting tests in this way. It's not easy to use the utest module with async test suites. anyway it works... 0_0

@arseny034
Copy link

Hey, how is it going? Should we expect these changes to be merged?

@wellwelwel wellwelwel added the needs typings Typings for new features label Apr 10, 2024
@wellwelwel
Copy link
Collaborator

wellwelwel commented Apr 10, 2024

Thanks, @fenying 🙋🏻‍♂️

Recently (#2472), we refactored all the tests.

Now, all test files need to have extensions as *.test.cjs (/test/{unit,integration}/**) or *.test.mjs (/test/esm/**).

@fenying
Copy link
Author

fenying commented Apr 10, 2024

Thanks, @fenying 🙋🏻‍♂️

Recently (#2472) we have refactored all tests.

Now, all test files need to have extensions as *.test.cjs (/test/{unit,integration}/**) or *.test.mjs (/test/esm/**) 🙋🏻‍♂️

I see, I am rechecking the contribution docs, and I will fix all these problems later. :)

@wellwelwel
Copy link
Collaborator

wellwelwel commented Apr 10, 2024

@fenying, in test/tsc-build/mysql/createPool/callbacks/createPool.test.ts, you don't need this 🙋🏻‍♂️

- pool = mysql.createPool({
-   ...access,
-   acquireTimeout: 1000,
- });

In test/integration/test-pool-acquire-timeout.test.js, you can change the file name to test/integration/test-pool-acquire-timeout.test.cjs 🧙🏻


For lint errors, I recommend you check it by running npm run lint locally.

@fenying
Copy link
Author

fenying commented Apr 10, 2024

@wellwelwel Ok, I missed the npm run lint:xxxx...

Now I have run every test and lint locally, except for test:bun becuase I don't have a bun env, and it looks running well in previous CI result.

There are still some other failed tests, but seem not related to these changes.

Copy link

codecov bot commented Apr 10, 2024

Codecov Report

Attention: Patch coverage is 89.28571% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 90.32%. Comparing base (cf3fa60) to head (5fabc6e).

Files Patch % Lines
lib/pool.js 86.95% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1919   +/-   ##
=======================================
  Coverage   90.32%   90.32%           
=======================================
  Files          71       71           
  Lines       15717    15740   +23     
  Branches     1333     1341    +8     
=======================================
+ Hits        14196    14217   +21     
- Misses       1521     1523    +2     
Flag Coverage Δ
compression-0 90.32% <89.28%> (+<0.01%) ⬆️
compression-1 90.32% <89.28%> (+<0.01%) ⬆️
tls-0 89.84% <89.28%> (+<0.01%) ⬆️
tls-1 90.14% <89.28%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wellwelwel wellwelwel added mysqljs-mysql-incompatibilities Previously: feligxe-mysql-incompatibilities and removed needs typings Typings for new features labels Apr 10, 2024
@wellwelwel

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mysqljs-mysql-incompatibilities Previously: feligxe-mysql-incompatibilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants