Sample Data and Docker shells #3277
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3277 +/- ##
====================================================
+ Coverage 48.04% 48.91% +0.87%
====================================================
Files 455 457 +2
Lines 33942 34152 +210
Branches 892 959 +67
====================================================
+ Hits 16308 16707 +399
+ Misses 17634 17445 -189 ☔ View full report in Codecov by Sentry. |
|
@palisadoes This is ready for review |
REGARDING VITESTSir since each contributor has there own way of writing tests, there are chances in future that some tests conflicts unexpectedly since integration tests are performed on actual database, like happened after last PR To reduce this I have enabled single threaded vitest flow so that integration tests don't conflict. Also if this problem comes in future, please suggest contributors to use database transactions so that after their tests are completed, database is rolled back to previous state ensuring other tests can run easily without conflicts, I have applied same to these tests, I hope they don't cause problem but if they please tag me I'll review right away, not only this, if there is any error regarding vitests conflicts you can tag me I have got some good understanding of our test environment and integration conflicts, I'll be happy to help. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
test/scripts/dbManagement/resetDB.test.ts (1)
25-40: 🛠️ Refactor suggestionAdd error handling for beforeEach and afterEach hooks.
The setup and teardown hooks should handle potential failures in database transactions and administrator creation.
beforeEach(async () => { vi.resetModules(); - await helpers.db.transaction(async (trx) => { - console.log("created transaction"); - await trx.execute(sql`BEGIN;`); - }); - await helpers.ensureAdministratorExists(); + try { + await helpers.db.transaction(async (trx) => { + await trx.execute(sql`BEGIN;`); + }); + await helpers.ensureAdministratorExists(); + } catch (error) { + console.error('Failed to setup test environment:', error); + throw error; + } }); afterEach(async () => { vi.restoreAllMocks(); - await helpers.ensureAdministratorExists(); - await helpers.db.transaction(async (trx) => { - await trx.execute(sql`ROLLBACK;`); - console.log("rolledback"); - }); + try { + await helpers.ensureAdministratorExists(); + await helpers.db.transaction(async (trx) => { + await trx.execute(sql`ROLLBACK;`); + }); + } catch (error) { + console.error('Failed to cleanup test environment:', error); + throw error; + } });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
test/scripts/dbManagement/addSampleData.test.ts(1 hunks)test/scripts/dbManagement/helpers.test.ts(1 hunks)test/scripts/dbManagement/resetDB.test.ts(1 hunks)vitest.config.ts(2 hunks)
🔇 Additional comments (7)
vitest.config.ts (1)
13-13: Consider making the timeout configurable.A 15-second timeout might be insufficient for large integration tests. Exposing this timeout as an environment variable or configuration property can provide flexibility across different test scenarios.
test/scripts/dbManagement/resetDB.test.ts (1)
42-139: LGTM! Comprehensive test coverage.The test suite provides excellent coverage of various scenarios including:
- Happy path with successful execution
- User cancellation
- Database connection failures
- Formatting failures
- Administrator creation failures
- Module import behavior
The tests are well-structured with proper mocking and assertions.
test/scripts/dbManagement/addSampleData.test.ts (3)
14-14: Fix typo in administrator email address.There's a typo in the mocked administrator email address.
56-68: Add timeout for concurrent operation test.The concurrent operation test might need a timeout to prevent hanging in case of deadlocks.
70-122: Well-structured error handling tests!The error handling tests are comprehensive, with good use of spies and clear error messages.
test/scripts/dbManagement/helpers.test.ts (2)
19-19: Fix typo in administrator email address.There's a typo in the mocked administrator email address.
89-110: Excellent date parsing test coverage!The tests cover various date formats, edge cases, and invalid inputs comprehensively.
There was a problem hiding this comment.
It appears that you have to do this so that any user running the docker container can get access to the fnm path.
Please move this modification to the very end of the file with an explanation in the comments. We need to ensure that this doesn't impact the starting of the containers in any way.
Validate that both the production and development containers start and can import the data after this change.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/docs/docs/getting-started/installation.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/docs/docs/getting-started/installation.md
401-401: Trailing spaces
Expected: 0 or 2; Actual: 3
(MD009, no-trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (2)
docs/docs/docs/getting-started/installation.md (2)
401-401: Trailing Spaces Detected
Static analysis flagged trailing spaces on this line. Please remove the extra whitespace to comply with markdown lint rules (MD009).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
401-401: Trailing spaces
Expected: 0 or 2; Actual: 3(MD009, no-trailing-spaces)
404-415: Updated VS Code Dev Container Instructions: Environment Clarity
The "Using VS Code Dev Container" section now directs users to import sample data using the commandpnpm run add:sample_data. This is clear and aligns with the new script naming. Consider adding a short note to remind users to ensure that their terminal session within the container has the correct environment variables loaded.
palisadoes
added
the
ignore-sensitive-files-pr
|
@palisadoes #3278 This will conflict with this PR since folders have been moved according to xoldd, what we can do is,
Meanwhile I will come up with a future proof solution for docker and vitest. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/docs/docs/developer-resources/testing.md(1 hunks)vitest.config.ts(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/docs/docs/developer-resources/testing.md
795-795: Multiple consecutive blank lines
Expected: 1; Actual: 2
(MD012, no-multiple-blanks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
vitest.config.ts (1)
13-13: Consider making the timeout configurable.A 15-second timeout might be insufficient for database operations, especially for sample data scripts. Consider exposing this timeout as an environment variable or configuration property.
|
|
||
| Now you can resume your development work. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhancement & Markdown Formatting Adjustment:
The concluding statement "Now you can resume your development work." serves as a clear directive for users after restarting the Docker environment, which is a helpful addition. However, static analysis indicates that there are multiple consecutive blank lines before this message (MD012). It is recommended to remove the extra blank line so that only a single blank line separates the previous code block from this concluding text.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
795-795: Multiple consecutive blank lines
Expected: 1; Actual: 2
(MD012, no-multiple-blanks)
| @@ -21,6 +21,6 @@ export default defineConfig({ | |||
| // teardownTimeout: 10000 | |||
|
|
|||
| hookTimeout: 30000, // 30 seconds for hooks | |||
| pool: "threads", // for faster test execution and to avoid postgres max-limit error | |||
| pool: "threads", | |||
There was a problem hiding this comment.
Verify the thread pool configuration.
The PR objectives mention enabling single-threaded flow to prevent test conflicts, but the current configuration pool: "threads" enables multi-threading. This could lead to integration test conflicts when multiple tests modify the same database state.
Apply this diff to enable single-threaded execution:
- pool: "threads",
+ poolOptions: {
+ // Force single-threaded execution to prevent conflicts in integration tests
+ // that modify the same database state
+ threads: {
+ singleThread: true
+ }
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pool: "threads", | |
| poolOptions: { | |
| // Force single-threaded execution to prevent conflicts in integration tests | |
| // that modify the same database state | |
| threads: { | |
| singleThread: true | |
| } | |
| }, |
Closes #3214
Description
Following can be used to at ease now as one line commands directly from cli:
docker exec talawa-api-1 /bin/bash -l -c '. pnpm run add:sample_data && exit'docker exec talawa-api-1 /bin/sh -l -c 'pnpm run add:sample_data && exit'docker exec talawa-api-1 /bin/zsh -l -c 'pnpm run add:sample_data && exit'Flow of script
add:sample_data
Proper error catching is implemented for best UX.
reset:db
Key Changes
Next Steps
Improve code coverage for scripts.
Summary by CodeRabbit
Documentation
New Features
Chores
Tests