[FEATURE] Add unit tests for core functionality - #934
Conversation
👷 Deploy request for fixmybharat pending review.Visit the deploys page to approve it
|
🙏 Thank you for your contribution, @rohitkumarnaidu!PR Details:
Quality Checklist:
Review Process:
Note: The maintainers will monitor code quality and ensure the overall project flow isn't broken. |
📝 WalkthroughWalkthroughThe TrendAnalyzer tests improve temporary SQLite database handling by adding pre-test cleanup, sequencing database closure after statement finalization, and delaying post-test file deletion with defensive error handling. ChangesTrendAnalyzer test database lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
👋 Hello @rohitkumarnaidu! Thank you for your first Pull Request to VishwaGuru! 🎉
We appreciate your contribution to this open source platform empowering India's youth to engage with democracy.
What happens next?
- A maintainer will review your PR soon
- Please ensure all tests pass and the code follows our style guidelines
- Be ready to address any feedback or requested changes
Before your PR is merged:
- Ensure your code follows the project's coding standards
- All tests should pass
- Update documentation if needed
- Make sure your commits are clear and descriptive
Resources:
Thank you for contributing! 🚀
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/dailyRefinement.test.ts`:
- Around line 17-19: Update both database cleanup sites in
tests/dailyRefinement.test.ts: in beforeAll, let unlinkSync failures propagate
unless the error code is ENOENT; in afterAll, explicitly close the TrendAnalyzer
callback-backed database before unlinking, remove the 500ms timer workaround,
and propagate any non-ENOENT unlinkSync failure.
- Around line 31-33: Update the beforeAll fixture-seeding flow in
tests/dailyRefinement.test.ts, including the db.run and stmt.run operations
around stmt.finalize, to propagate each operation’s first error to Jest’s done
callback and stop the pipeline on failure. Only invoke db.close(done) after
table creation and inserts complete successfully, preserving the existing
successful cleanup behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ab9f408e-e5e5-4cc6-9b14-8a57eeea6212
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
tests/dailyRefinement.test.ts
| if (fs.existsSync(dbPath)) { | ||
| try { fs.unlinkSync(dbPath); } catch (e) {} | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)dailyRefinement\.test\.ts$|dailyRefinement|issues\.db|test_issues\.db' || true
echo "== target file outline/size =="
if [ -f tests/dailyRefinement.test.ts ]; then
wc -l tests/dailyRefinement.test.ts
ast-grep outline tests/dailyRefinement.test.ts --view compact || true
echo "== target file relevant sections =="
sed -n '1,90p' tests/dailyRefinement.test.ts | cat -n
fi
echo "== SQLite/database references in tests =="
rg -n "test_issues\.db|createClient|close|unlinkSync|existsSync|dbPath|sqlite|SQLite|dailyRefinement" tests src . 2>/dev/null | head -200 || trueRepository: RohanExploit/VishwaGuru
Length of output: 16083
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== trendAnalyzer outline =="
ast-grep outline services/trendAnalyzer.ts --view signatures || true
echo "== trendAnalyzer relevant sections =="
sed -n '1,110p' services/trendAnalyzer.ts | cat -n
echo "== package test scripts =="
cat package.json | jq '{scripts: .scripts, dependencies: {sqlite3:.dependencies.sqlite3}, devDependencies:.devDependencies}'Repository: RohanExploit/VishwaGuru
Length of output: 3607
Do not silently ignore SQLite cleanup failures.
Both lifecycle boundaries can leave test_issues.db behind while the test suite still reports success:
beforeAll: fail setup on anyunlinkSyncerror exceptENOENTinstead of proceeding with a corrupted/stale database.afterAll: close theTrendAnalyzercallback-backed database before unlinking and propagate non-ENOENTcleanup errors instead of relying on a 500ms timer.
📍 Affects 1 file
tests/dailyRefinement.test.ts#L17-L19(this comment)tests/dailyRefinement.test.ts#L37-L43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/dailyRefinement.test.ts` around lines 17 - 19, Update both database
cleanup sites in tests/dailyRefinement.test.ts: in beforeAll, let unlinkSync
failures propagate unless the error code is ENOENT; in afterAll, explicitly
close the TrendAnalyzer callback-backed database before unlinking, remove the
500ms timer workaround, and propagate any non-ENOENT unlinkSync failure.
| stmt.finalize(() => { | ||
| db.close(done); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a dailyRefinement.test.ts . || true
echo "== inspect file around relevant lines =="
if [ -f tests/dailyRefinement.test.ts ]; then
cat -n tests/dailyRefinement.test.ts | sed -n '1,120p'
fi
echo "== package hints for sqlite dependency =="
for f in package.json pnpm-lock.yaml package-lock.json yarn.lock; do
[ -f "$f" ] && { echo "--- $f"; rg -n "sqlite3|better-sqlite3|node-sqlite3" "$f" || true; }
doneRepository: RohanExploit/VishwaGuru
Length of output: 5816
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect whether node-sqlite3's API signature for CREATE TABLE/run/close/use callbacks is present in the repository
# as installed docs or examples, and compare common callback behavior from local package metadata/docs if present.
python3 - <<'PY'
from pathlib import Path
paths=[]
for root in [Path('.')]:
for p in root.rglob('*sqlite3*'):
if p.is_file():
paths.append(str(p))
print("sqlite3-related files:", "\n".join(paths[:20]) if paths else "(none)")
for p in [Path('node_modules/sqlite3/package.json'), Path('node_modules/sqlite3/LICENSE.md'), Path('node_modules/better-sqlite3/package.json')]:
if p.exists():
print(f"--- {p} first 20 lines")
lines=p.read_text(encoding='utf-8', errors='replace').splitlines()[:20]
print("\n".join(lines))
PY
echo "== any local documentation examples =="
rg -n "stmt\\.run|CREATE TABLE|db\\.close|db\\.serialize|node-sqlite3|better-sqlite3" . \
-g '!node_modules' -g '!dist' -g '!build' -g '!coverage' \
-C 3 || trueRepository: RohanExploit/VishwaGuru
Length of output: 11639
🌐 Web query:
node-sqlite3 v6 Database.prototype run Statement.prototype run finalize close callback error signature
💡 Result:
In the node-sqlite3 library, the callback error signature for Database#run, Statement#run, Database#close, and Statement#finalize generally follows the standard Node.js convention where the first argument is an error object (or null if no error occurred) [1][2][3]. Specifically, the callback signatures are as follows: Database.prototype.run and Statement.prototype.run The callback signature is function(err). If an error occurs, the first parameter is an Error object; if execution is successful, the first parameter is null [1][2]. Note that unlike some other Node.js libraries, result data for run is not passed as an argument to the callback; instead, relevant information (such as changes or lastID) is accessed via the 'this' context of the callback function [1][4]. Because of this binding, you should avoid using arrow functions for these callbacks if you need to access that context, as arrow functions lexically bind 'this' [5][4]. Database.prototype.close The callback signature is function(err). The err parameter will be an Error object if the database fails to close, or null if it closes successfully [2][6]. Statement.prototype.finalize The callback signature is function(err). Similar to the others, the first parameter is an Error object if an error occurs during finalization, or null otherwise [2]. Summary of callback error behavior: - If a callback is provided, errors are passed as the first argument [1][3]. - If no callback is provided, errors are typically emitted as 'error' events on the database or statement object [1][7][3]. - For run specifically, remember that data is returned via the 'this' object, not as a second argument to the callback [1][4].
Citations:
- 1: https://github.com/TryGhost/node-sqlite3/wiki/API
- 2: https://github.com/TryGhost/node-sqlite3/blob/master/lib/sqlite3.d.ts
- 3: https://stackoverflow.com/questions/32980459/error-handling-in-node-sqlite3
- 4: odyssee of a noob trying to get the ID of inserted row TryGhost/node-sqlite3#622
- 5: https://stackoverflow.com/questions/10022984/nodejs-node-sqlite3-run-callback-not-working
- 6:
db.close()never finishes on a DB with invalid path TryGhost/node-sqlite3#1617 - 7: https://github.com/mapbox/node-sqlite3/blob/master/src/statement.cc
Propagate fixture-seeding errors into Jest.
db.run and stmt.run callback on errors, but these calls omit callbacks so table creation or inserts can fail without calling beforeAll’s done; only db.close(done) reports the close operation. Add callbacks for these setup operations that pass the first error to done, and only close when the seeding pipeline succeeds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/dailyRefinement.test.ts` around lines 31 - 33, Update the beforeAll
fixture-seeding flow in tests/dailyRefinement.test.ts, including the db.run and
stmt.run operations around stmt.finalize, to propagate each operation’s first
error to Jest’s done callback and stop the pipeline on failure. Only invoke
db.close(done) after table creation and inserts complete successfully,
preserving the existing successful cleanup behavior.
Source: MCP tools
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/dailyRefinement.test.ts">
<violation number="1" location="tests/dailyRefinement.test.ts:18">
P2: Silently catching all exceptions from `unlinkSync` can hide real problems. If the file exists but removal fails for a reason other than `ENOENT` (e.g., permission denied, file locked), the test will proceed with a stale database and may produce misleading results. Consider re-throwing non-`ENOENT` errors:
```ts
try { fs.unlinkSync(dbPath); } catch (e: any) {
if (e.code !== 'ENOENT') throw e;
}
The same applies to the afterAll cleanup block.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| try { fs.unlinkSync(dbPath); } catch (e) {} | ||
| } | ||
| done(); | ||
| }, 500); // wait for locks to release |
There was a problem hiding this comment.
P2: Teardown is still racy: a fixed delay does not prove that SQLite released the lock, and cleanup failure can be hidden after the delay. Awaiting the analyzer/database close completion and reporting or retrying unlink errors would prevent leaked test artifacts and cross-run contamination.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/dailyRefinement.test.ts, line 43:
<comment>Teardown is still racy: a fixed delay does not prove that SQLite released the lock, and cleanup failure can be hidden after the delay. Awaiting the analyzer/database close completion and reporting or retrying unlink errors would prevent leaked test artifacts and cross-run contamination.</comment>
<file context>
@@ -25,12 +28,19 @@ describe('Daily Civic Intelligence Refinement Engine', () => {
+ try { fs.unlinkSync(dbPath); } catch (e) {}
+ }
+ done();
+ }, 500); // wait for locks to release
});
</file context>
| stmt.finalize(() => { | ||
| db.close(done); | ||
| }); |
There was a problem hiding this comment.
P2: Database setup can report success even when statement finalization fails, leaving incomplete seed data and making the test pass or fail for the wrong reason. Preserving the finalize error before closing the database keeps setup failures visible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/dailyRefinement.test.ts, line 31:
<comment>Database setup can report success even when statement finalization fails, leaving incomplete seed data and making the test pass or fail for the wrong reason. Preserving the finalize error before closing the database keeps setup failures visible.</comment>
<file context>
@@ -25,12 +28,19 @@ describe('Daily Civic Intelligence Refinement Engine', () => {
stmt.run('Another pothole here', 'infrastructure', 'Ward 1', now);
- stmt.finalize(done);
+ stmt.finalize(() => {
+ db.close(done);
+ });
</file context>
| stmt.finalize(() => { | |
| db.close(done); | |
| }); | |
| stmt.finalize((err) => { | |
| if (err) return done(err); | |
| db.close(done); | |
| }); |
| beforeAll((done) => { | ||
| dbPath = path.resolve(__dirname, 'test_issues.db'); | ||
| if (fs.existsSync(dbPath)) { | ||
| try { fs.unlinkSync(dbPath); } catch (e) {} |
There was a problem hiding this comment.
P2: Silently catching all exceptions from unlinkSync can hide real problems. If the file exists but removal fails for a reason other than ENOENT (e.g., permission denied, file locked), the test will proceed with a stale database and may produce misleading results. Consider re-throwing non-ENOENT errors:
try { fs.unlinkSync(dbPath); } catch (e: any) {
if (e.code !== 'ENOENT') throw e;
}The same applies to the afterAll cleanup block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/dailyRefinement.test.ts, line 18:
<comment>Silently catching all exceptions from `unlinkSync` can hide real problems. If the file exists but removal fails for a reason other than `ENOENT` (e.g., permission denied, file locked), the test will proceed with a stale database and may produce misleading results. Consider re-throwing non-`ENOENT` errors:
```ts
try { fs.unlinkSync(dbPath); } catch (e: any) {
if (e.code !== 'ENOENT') throw e;
}
The same applies to the afterAll cleanup block.
|
Closing old duplicate - superseded by PR #937. |
Fixes #925. Added unit tests for core functionality and fixed db locking issue.
Summary by cubic
Adds unit tests for the Daily Civic Intelligence Refinement Engine and fixes test-time
sqlite3DB locking to stabilize CI. Fixes #925.New Features
tests/dailyRefinement.test.tsthat seed issues and verify 24-hour trend analysis.Bug Fixes
Written for commit b9c6a3d. Summary will update on new commits.
Summary by CodeRabbit