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

sqlite: split up large test file #54014

Closed
wants to merge 3 commits into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jul 24, 2024

The original test/parallel/test-sqlite.js test appears to time out in the CI occasionally. This commit splits the test into several smaller test files.

Fixes: #54006

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jul 24, 2024
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 24, 2024

Weird. Now Windows is failing because the tmp directory cleanup is failing with EPERM.

@richardlau
Copy link
Member

Weird. Now Windows is failing because the tmp directory cleanup is failing with EPERM.

I'm a bit worried that #53617 may have introduced a subtle change in behaviour/regression.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

cjihrig added a commit to cjihrig/node that referenced this pull request Jul 25, 2024
The current test is large and can time out. It should be split
into multiple smaller tests as done in nodejs#54014. However, that
approach appears to change GC behavior such that the database
files are not cleaned up quickly enough on Windows. Forcing
any unfinalized SQL statements to be GC'ed appears to fix the
problem. Mark the original test as flaky until the necessary
code changes are made.
@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 25, 2024

The issue with these smaller tests appears to be related to relying on GC to finalize the prepared statements and close the database connections. Fixing it properly will take a little more time, so I have opened #54031 to mark the original test as flaky in the mean time.

nodejs-github-bot pushed a commit that referenced this pull request Jul 25, 2024
The current test is large and can time out. It should be split
into multiple smaller tests as done in #54014. However, that
approach appears to change GC behavior such that the database
files are not cleaned up quickly enough on Windows. Forcing
any unfinalized SQL statements to be GC'ed appears to fix the
problem. Mark the original test as flaky until the necessary
code changes are made.

PR-URL: #54031
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
targos pushed a commit that referenced this pull request Jul 28, 2024
The current test is large and can time out. It should be split
into multiple smaller tests as done in #54014. However, that
approach appears to change GC behavior such that the database
files are not cleaned up quickly enough on Windows. Forcing
any unfinalized SQL statements to be GC'ed appears to fix the
problem. Mark the original test as flaky until the necessary
code changes are made.

PR-URL: #54031
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 5, 2024
The current test is large and can time out. It should be split
into multiple smaller tests as done in #54014. However, that
approach appears to change GC behavior such that the database
files are not cleaned up quickly enough on Windows. Forcing
any unfinalized SQL statements to be GC'ed appears to fix the
problem. Mark the original test as flaky until the necessary
code changes are made.

PR-URL: #54031
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 6, 2024

There have been so many problems lately with EPERM on Windows that I'm not sure the failures on Windows were ever related to the changes in this PR. Starting the CI all over again to see how it goes.

@cjihrig cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 6, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 6, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link

codecov bot commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 73.58491% with 14 lines in your changes missing coverage. Please review.

Project coverage is 87.10%. Comparing base (e0634f5) to head (ee5b9c1).
Report is 459 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 73.58% 1 Missing and 13 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54014      +/-   ##
==========================================
- Coverage   87.10%   87.10%   -0.01%     
==========================================
  Files         647      647              
  Lines      181739   181775      +36     
  Branches    34887    34897      +10     
==========================================
+ Hits       158310   158337      +27     
+ Misses      16738    16736       -2     
- Partials     6691     6702      +11     
Files with missing lines Coverage Δ
src/node_sqlite.h 0.00% <ø> (ø)
src/node_sqlite.cc 82.34% <73.58%> (-0.38%) ⬇️

... and 22 files with indirect coverage changes

@RedYetiDev RedYetiDev added the sqlite Issues and PRs related to the SQLite subsystem. label Aug 6, 2024
@cjihrig cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 7, 2024

I've made the changes I outlined in #54014 (comment). This is now more than just a test change. The CI seems happy (still resuming some unrelated failures). I would appreciate re-reviews.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 8, 2024

@cjihrig cjihrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Aug 12, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in 0301309...b8a2550

nodejs-github-bot pushed a commit that referenced this pull request Aug 12, 2024
This commit adds statement tracking to the DatabaseSync class.
When a database is closed manually or via garbage collection, it
will force all associated prepared statements to be finalized.
This should mitigate "zombie" connections which can introduce
test flakiness in the CI on Windows.

PR-URL: #54014
Fixes: #54006
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Aug 12, 2024
The original test/parallel/test-sqlite.js test appears to time
out in the CI occasionally. This commit splits the test into
several smaller test files.

Fixes: #54006
PR-URL: #54014
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Aug 12, 2024
PR-URL: #54014
Fixes: #54006
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@cjihrig cjihrig deleted the split-up-sqlite-test branch August 12, 2024 13:53
targos pushed a commit that referenced this pull request Aug 14, 2024
This commit adds statement tracking to the DatabaseSync class.
When a database is closed manually or via garbage collection, it
will force all associated prepared statements to be finalized.
This should mitigate "zombie" connections which can introduce
test flakiness in the CI on Windows.

PR-URL: #54014
Fixes: #54006
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Aug 14, 2024
The original test/parallel/test-sqlite.js test appears to time
out in the CI occasionally. This commit splits the test into
several smaller test files.

Fixes: #54006
PR-URL: #54014
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Aug 14, 2024
PR-URL: #54014
Fixes: #54006
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Aug 14, 2024
This commit adds statement tracking to the DatabaseSync class.
When a database is closed manually or via garbage collection, it
will force all associated prepared statements to be finalized.
This should mitigate "zombie" connections which can introduce
test flakiness in the CI on Windows.

PR-URL: #54014
Fixes: #54006
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Aug 14, 2024
The original test/parallel/test-sqlite.js test appears to time
out in the CI occasionally. This commit splits the test into
several smaller test files.

Fixes: #54006
PR-URL: #54014
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Aug 14, 2024
PR-URL: #54014
Fixes: #54006
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Aug 19, 2024
The current test is large and can time out. It should be split
into multiple smaller tests as done in #54014. However, that
approach appears to change GC behavior such that the database
files are not cleaned up quickly enough on Windows. Forcing
any unfinalized SQL statements to be GC'ed appears to fix the
problem. Mark the original test as flaky until the necessary
code changes are made.

PR-URL: #54031
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Aug 19, 2024
The current test is large and can time out. It should be split
into multiple smaller tests as done in #54014. However, that
approach appears to change GC behavior such that the database
files are not cleaned up quickly enough on Windows. Forcing
any unfinalized SQL statements to be GC'ed appears to fix the
problem. Mark the original test as flaky until the necessary
code changes are made.

PR-URL: #54031
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Aug 19, 2024
The current test is large and can time out. It should be split
into multiple smaller tests as done in #54014. However, that
approach appears to change GC behavior such that the database
files are not cleaned up quickly enough on Windows. Forcing
any unfinalized SQL statements to be GC'ed appears to fix the
problem. Mark the original test as flaky until the necessary
code changes are made.

PR-URL: #54031
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. sqlite Issues and PRs related to the SQLite subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky: parallel.test-sqlite
9 participants