FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

sqlite: split up large test file by cjihrig · Pull Request #54014 · nodejs/node · GitHub

/ node Public

sqlite: split up large test file - #54014

Closed
cjihrig wants to merge 3 commits into
nodejs:mainfrom
cjihrig:split-up-sqlite-test
Closed

sqlite: split up large test file#54014
cjihrig wants to merge 3 commits into
nodejs:mainfrom
cjihrig:split-up-sqlite-test

Conversation

cjihrig commented Jul 24, 2024

Copy link
Copy Markdown
Contributor

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 added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jul 24, 2024
MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2024

Copy link
Copy Markdown
Collaborator

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

Copy link
Copy Markdown
Collaborator

cjihrig commented Jul 24, 2024

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
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 commented Jul 25, 2024
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 <moshe@atlow.co.il>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
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 <moshe@atlow.co.il>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
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 <moshe@atlow.co.il>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
cjihrig force-pushed the split-up-sqlite-test branch from ab6834b to 31e8790 Compare August 6, 2024 13:36

cjihrig commented Aug 6, 2024

Copy link
Copy Markdown
Contributor Author

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 added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 6, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 6, 2024

Copy link
Copy Markdown
Collaborator

codecov Bot commented Aug 6, 2024
edited
Loading

Copy link
Copy Markdown

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

avivkeller added the sqlite Issues and PRs related to the SQLite subsystem. label Aug 6, 2024
cjihrig force-pushed the split-up-sqlite-test branch from 3ca154d to ee5b9c1 Compare August 7, 2024 13:52
cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2024

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

cjihrig commented Aug 7, 2024

Copy link
Copy Markdown
Contributor Author

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 commented Aug 8, 2024
edited by cjihrig
Loading

Copy link
Copy Markdown
Collaborator

cjihrig added author ready PRs that have at least one approval, no outstanding review comments, 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 removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2024

Copy link
Copy Markdown
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 <moshe@atlow.co.il>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
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 <moshe@atlow.co.il>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Aug 12, 2024
PR-URL: #54014
Fixes: #54006
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
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 <moshe@atlow.co.il>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
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 <moshe@atlow.co.il>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request Aug 14, 2024
PR-URL: #54014
Fixes: #54006
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
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 <moshe@atlow.co.il>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
RafaelGSS mentioned this pull request Aug 19, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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 outstanding review comments, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. 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


Back | FazBrowse Home | New Git URL