optimize tests run time#2039
Conversation
update branch to latest
|
""" WalkthroughA new test execution strategy document was added, and the test scripts in Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant NPM
participant Mocha
participant Database
Developer->>NPM: Run "npm test"
NPM->>NPM: Run "test:setup"
NPM->>NPM: Run "test:sequential"
NPM->>Mocha: Execute repository tests sequentially
Mocha->>Database: Modify shared state (sequentially)
NPM->>NPM: Run "test:parallel"
NPM->>Mocha: Execute other tests in parallel groups
Mocha->>Database: Access (read/isolated writes)
Mocha->>Developer: Return test results
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
package.json (1)
143-143: Duplicate script name – pick one canonical alias
"test:project-entity"duplicates the functionality of existing"test:projectEntity"(line 126). Having two entry points that do the same thing is confusing and fragile when one gets out of sync.Delete one or add a comment declaring which is canonical.
TEST_STRATEGY.md (2)
42-55: Add fenced-code languages to satisfy markdown-lint
markdownlintflags MD040 because the two fenced blocks don’t specify a language.
Simply appendbash(orsh) to both triple-backtick openings:-``` +npm +```bash npm test...
-+npm +bash
npm run test:sequential # All repository tests in sequenceThis keeps automated doc checks green. --- `64-68`: **Highlight cleanup requirement between phases** The doc states that the parallel phase starts “after the sequential phase completes”, but omits the practical step of cleaning / reseeding the DB to avoid leftover state leaking into parallel workers. Add a short note such as: > “Before launching the parallel phase, the test harness resets the database by running `npm run db:migrate:revert:test && npm run db:migrate:run:test`.” This makes the strategy self-contained and prevents mis-implementation. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 659825109a999f821e1259e198f767a7cd1c48da and 36da1ad707aee70d8b0c06ee8bb645d2a9ccd811. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `TEST_STRATEGY.md` (1 hunks) * `package.json` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>TEST_STRATEGY.md</summary> 42-42: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 49-49: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)</summary> * GitHub Check: test * GitHub Check: Analyze (javascript-typescript) </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| "test:power": "NODE_ENV=test mocha -t 90000 ./test/pre-test-scripts.ts './src/repositories/powerBoostingRepository.test.ts' './src/repositories/powerBalanceSnapshotRepository.test.ts' './src/repositories/powerSnapshotRepository.test.ts' './src/repositories/powerRoundRepository.test.ts' './src/repositories/instantBoostingRepository.test.ts' './src/repositories/projectPowerViewRepository.test.ts' './src/repositories/userProjectPowerViewRepository.test.ts' './src/repositories/previousRoundRankRepository.test.ts'", | ||
| "test:project-repos": "NODE_ENV=test mocha -t 90000 ./test/pre-test-scripts.ts './src/repositories/projectRepository.test.ts' './src/repositories/donationRepository.test.ts' './src/repositories/projectAddressRepository.test.ts' './src/repositories/projectUpdateRepository.test.ts' './src/repositories/projectVerificationRepository.test.ts' './src/repositories/draftDonationRepository.test.ts' './src/repositories/recurringDonationRepository.test.ts'", | ||
| "test:other-repos": "NODE_ENV=test mocha -t 90000 ./test/pre-test-scripts.ts './src/repositories/anchorContractAddressRepository.test.ts' './src/repositories/broadcastNotificationRepository.test.ts' './src/repositories/campaignRepository.test.ts' './src/repositories/causeProjectRepository.test.ts' './src/repositories/causeRepository.test.ts' './src/repositories/dbCronRepository.test.ts' './src/repositories/projectGivbackViewRepository.test.ts' './src/repositories/qfRoundHistoryRepository.test.ts' './src/repositories/qfRoundRepository.test.ts' './src/repositories/sitemapUrlRepository.test.ts' './src/repositories/socialProfileRepository.test.ts' './src/repositories/statusReasonRepository.test.ts'", | ||
| "test:syncProjectsRequiredForListing": "NODE_ENV=test mocha ./test/pre-test-scripts.ts ./src/services/cronJobs/syncProjectsRequiredForListing.test.ts", |
There was a problem hiding this comment.
Enforce isolated DB state for repository sub-groups
test:power, test:project-repos, and test:other-repos still run sequentially but reuse the same DB that test:sequential already mutated.
Failing to reset between calls will make these scripts non-deterministic when developers run them individually.
Add an explicit migration revert / re-seed before each script, e.g.
-"test:power": "NODE_ENV=test mocha -t 90000 ./test/pre-test-scripts.ts '...'",
+"test:power": "NODE_ENV=test npm run db:migrate:revert:test && npm run db:migrate:run:test && mocha -t 90000 ./test/pre-test-scripts.ts '...'",(or call a helper that wipes the database).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In package.json lines 120 to 123, the test scripts for different repository
groups reuse the same mutated database state from previous tests, causing
non-deterministic results. To fix this, modify each test script to include a
step that reverts migrations or reseeds the database before running the tests.
This can be done by adding a command or calling a helper script that wipes or
resets the database state explicitly before executing each test script.
| "test": "npm run test:sequential && npm run test:parallel", | ||
| "test:sequential": "NODE_ENV=test mocha -t 90000 ./test/pre-test-scripts.ts './src/repositories/**/*.test.ts'", | ||
| "test:parallel": "NODE_ENV=test mocha --parallel -t 90000 ./test/pre-test-scripts.ts './src/utils/**/*.test.ts' './src/middleware/**/*.test.ts' './src/graphql/**/*.test.ts' './src/apollo/**/*.test.ts' './src/auth/**/*.test.ts' './src/controllers/**/*.test.ts' './src/entities/**/*.test.ts' './src/server/**/*.test.ts' './src/services/**/*.test.ts' './src/adapters/**/*.test.ts' './src/resolvers/**/*.test.ts' './migration/tests/migration.test.ts'", | ||
| "test:power": "NODE_ENV=test mocha -t 90000 ./test/pre-test-scripts.ts './src/repositories/powerBoostingRepository.test.ts' './src/repositories/powerBalanceSnapshotRepository.test.ts' './src/repositories/powerSnapshotRepository.test.ts' './src/repositories/powerRoundRepository.test.ts' './src/repositories/instantBoostingRepository.test.ts' './src/repositories/projectPowerViewRepository.test.ts' './src/repositories/userProjectPowerViewRepository.test.ts' './src/repositories/previousRoundRankRepository.test.ts'", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider limiting parallel worker count and making timeout configurable
mocha --parallel will spin up as many workers as CPUs by default. On CI runners with limited memory this frequently ends in OOM or DB-connection exhaustion. Mocha exposes --jobs <n> for taming this.
-"test:parallel": "NODE_ENV=test mocha --parallel -t 90000 ...
+"test:parallel": "NODE_ENV=test mocha --parallel --jobs 4 -t ${PARALLEL_TIMEOUT:-90000} ...• Adds a sensible default (4) yet allows callers to override with MOCHA_JOBS.
• Reads timeout from env so the same command works for local runs (shorter) and CI (longer).
📝 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.
| "test": "npm run test:sequential && npm run test:parallel", | |
| "test:sequential": "NODE_ENV=test mocha -t 90000 ./test/pre-test-scripts.ts './src/repositories/**/*.test.ts'", | |
| "test:parallel": "NODE_ENV=test mocha --parallel -t 90000 ./test/pre-test-scripts.ts './src/utils/**/*.test.ts' './src/middleware/**/*.test.ts' './src/graphql/**/*.test.ts' './src/apollo/**/*.test.ts' './src/auth/**/*.test.ts' './src/controllers/**/*.test.ts' './src/entities/**/*.test.ts' './src/server/**/*.test.ts' './src/services/**/*.test.ts' './src/adapters/**/*.test.ts' './src/resolvers/**/*.test.ts' './migration/tests/migration.test.ts'", | |
| "test:power": "NODE_ENV=test mocha -t 90000 ./test/pre-test-scripts.ts './src/repositories/powerBoostingRepository.test.ts' './src/repositories/powerBalanceSnapshotRepository.test.ts' './src/repositories/powerSnapshotRepository.test.ts' './src/repositories/powerRoundRepository.test.ts' './src/repositories/instantBoostingRepository.test.ts' './src/repositories/projectPowerViewRepository.test.ts' './src/repositories/userProjectPowerViewRepository.test.ts' './src/repositories/previousRoundRankRepository.test.ts'", | |
| "test": "npm run test:sequential && npm run test:parallel", | |
| "test:sequential": "NODE_ENV=test mocha -t 90000 ./test/pre-test-scripts.ts './src/repositories/**/*.test.ts'", | |
| "test:parallel": "NODE_ENV=test mocha --parallel --jobs 4 -t ${PARALLEL_TIMEOUT:-90000} ./test/pre-test-scripts.ts './src/utils/**/*.test.ts' './src/middleware/**/*.test.ts' './src/graphql/**/*.test.ts' './src/apollo/**/*.test.ts' './src/auth/**/*.test.ts' './src/controllers/**/*.test.ts' './src/entities/**/*.test.ts' './src/server/**/*.test.ts' './src/services/**/*.test.ts' './src/adapters/**/*.test.ts' './src/resolvers/**/*.test.ts' './migration/tests/migration.test.ts'", | |
| "test:power": "NODE_ENV=test mocha -t 90000 ./test/pre-test-scripts.ts './src/repositories/powerBoostingRepository.test.ts' './src/repositories/powerBalanceSnapshotRepository.test.ts' './src/repositories/powerSnapshotRepository.test.ts' './src/repositories/powerRoundRepository.test.ts' './src/repositories/instantBoostingRepository.test.ts' './src/repositories/projectPowerViewRepository.test.ts' './src/repositories/userProjectPowerViewRepository.test.ts' './src/repositories/previousRoundRankRepository.test.ts'", |
🤖 Prompt for AI Agents
In package.json lines 117 to 120, the mocha test:parallel script runs with
default parallel workers equal to CPU count, which can cause OOM or DB
connection issues on CI. Modify the test:parallel script to include the --jobs
option with a default of 4 workers, allowing override via an environment
variable MOCHA_JOBS. Also, make the timeout configurable by reading it from an
environment variable with a fallback default, so local and CI runs can use
different timeouts without changing the script.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
test/pre-test-scripts.ts (1)
593-639: Ensure test setup runs in every Mocha workerWhen running
npm run test:parallel, Mocha spins up separate processes and only loads the spec files assigned to each worker—so placingpre-test-scripts.tsas a spec means most workers never execute yourbefore/beforeEachhooks. Switch to using Mocha’s--file(or a root-hook via--require) to guarantee the setup script is loaded in every worker.• Update package.json scripts:
–test:sequential
–test:parallel
• Update.mocharc.all-test.json: move the setup file out ofspecand intofile
• Remove./test/pre-test-scripts.tsfrom all spec listsSuggested diff:
--- a/package.json +++ b/package.json @@ scripts { - "test:sequential": "NODE_ENV=test mocha -t 90000 ./test/pre-test-scripts.ts './src/repositories/**/*.test.ts'", + "test:sequential": "NODE_ENV=test mocha --file ./test/pre-test-scripts.ts -t 90000 './src/repositories/**/*.test.ts'", @@ - "test:parallel": "NODE_ENV=test mocha --parallel -t 90000 ./test/pre-test-scripts.ts './src/utils/**/*.test.ts' '...other patterns...'", + "test:parallel": "NODE_ENV=test mocha --parallel --file ./test/pre-test-scripts.ts -t 90000 './src/utils/**/*.test.ts' '...other patterns...'",--- a/.mocharc.all-test.json +++ b/.mocharc.all-test.json @@ - "spec": [ - "./test/pre-test-scripts.ts", - "./src/**/*.test.ts", - "./src/**/**/*.test.ts", - "./migration/tests/*.test.ts" - ], + "file": ["./test/pre-test-scripts.ts"], + "spec": [ + "./src/**/*.test.ts", + "./src/**/**/*.test.ts", + "./migration/tests/*.test.ts" + ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/pre-test-scripts.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
test/pre-test-scripts.ts (2)
566-591: Database readiness verification implementation looks solid.The function properly checks both database initialization and connectivity with good error handling and logging.
The implementation correctly:
- Verifies AppDataSource initialization
- Tests connectivity with a simple query
- Provides detailed error messages
- Includes appropriate logging for debugging
593-632: Enhanced before hook improves test reliability.The detailed logging and explicit database readiness checks will help with debugging test setup issues. The 60-second timeout is reasonable for database initialization.
Good improvements:
- Comprehensive logging at each setup stage
- Explicit database readiness verification
- Proper error handling with descriptive messages
- Reasonable timeout for database operations
| // Global flag to track database initialization status | ||
| let databaseInitialized = false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider potential race conditions with global flag in parallel test execution.
The global flag approach may cause issues when tests run in parallel, as multiple test processes could interfere with each other's initialization state.
Consider using a more robust approach for tracking database initialization state:
-// Global flag to track database initialization status
-let databaseInitialized = false;
+// Use a more robust state management approach
+const testState = {
+ databaseInitialized: false,
+ initializationPromise: null as Promise<void> | null,
+};This allows for better coordination between parallel test processes and prevents race conditions.
📝 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.
| // Global flag to track database initialization status | |
| let databaseInitialized = false; | |
| // Use a more robust state management approach | |
| const testState = { | |
| databaseInitialized: false, | |
| initializationPromise: null as Promise<void> | null, | |
| }; |
🤖 Prompt for AI Agents
In test/pre-test-scripts.ts at lines 563-564, the use of a global flag to track
database initialization can cause race conditions during parallel test
execution. Replace the global boolean flag with a more robust synchronization
mechanism, such as using a file lock, a shared resource with atomic operations,
or a dedicated coordination service to ensure only one process initializes the
database at a time. This will prevent multiple test processes from interfering
with each other's initialization state.
| // Add a beforeEach hook to ensure database is ready before each test | ||
| beforeEach(function () { | ||
| if (!databaseInitialized) { | ||
| throw new Error('Database not initialized. Tests cannot run.'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test gating logic may not work effectively in parallel execution.
While the beforeEach hook provides a safety check, it may not be sufficient for the parallel test execution strategy mentioned in the PR objectives.
Consider a more sophisticated approach that accounts for parallel test execution:
-// Add a beforeEach hook to ensure database is ready before each test
-beforeEach(function () {
- if (!databaseInitialized) {
- throw new Error('Database not initialized. Tests cannot run.');
- }
-});
+// Add a beforeEach hook with better parallel execution support
+beforeEach(async function () {
+ if (!testState.databaseInitialized) {
+ // If initialization is in progress, wait for it
+ if (testState.initializationPromise) {
+ await testState.initializationPromise;
+ } else {
+ throw new Error('Database not initialized. Tests cannot run.');
+ }
+ }
+});This approach better handles scenarios where initialization might be in progress in another process.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/pre-test-scripts.ts around lines 634 to 639, the current beforeEach hook
throws an error if the database is not initialized, which may fail under
parallel test execution. To fix this, implement a synchronization mechanism such
as a shared promise or a locking strategy that waits for the database
initialization to complete before proceeding with each test. This ensures tests
do not fail prematurely when initialization is still in progress in another
parallel process.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/repositories/projectAddressRepository.ts (1)
47-79: Address inconsistency in case handling across chain types.The function now uses different casing strategies for different chain types:
- Solana:
UPPER()comparison- Stellar:
UPPER()comparison- EVM:
LOWER()comparisonWhile this might be intentional based on each chain's address format requirements, it creates inconsistency that could be confusing. Additionally, other functions in this file use different approaches (e.g.,
findAllRelatedAddressByWalletAddressusesLOWER()for all addresses).Consider standardizing the approach across all chain types or documenting the rationale for different casing strategies. Also ensure consistency with other functions in the same repository.
// Consider creating a helper function for address normalization const normalizeAddressForChain = (address: string, chainType: ChainType): string => { switch (chainType) { case ChainType.SOLANA: case ChainType.STELLAR: return address.toUpperCase(); case ChainType.EVM: default: return address.toLowerCase(); } };
🧹 Nitpick comments (1)
src/repositories/projectAddressRepository.ts (1)
49-50: Consider database performance impact of using UPPER() in WHERE clause.Using
UPPER(address)in the WHERE clause prevents the database from using a regular index on theaddresscolumn, which could impact query performance. This is especially important for frequently called repository functions.Consider one of these alternatives:
- Create a functional index on
UPPER(address)if case-insensitive matching is required- Store addresses in a consistent case format and use direct equality comparison
- Handle case normalization at the application layer before storing
-- Option 1: Create functional index (if case-insensitive matching is needed) CREATE INDEX idx_project_address_upper ON project_address (UPPER(address));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/repositories/projectAddressRepository.ts(1 hunks)src/utils/validators/projectValidator.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
| query = query.where(`UPPER(address) = :walletAddress`, { | ||
| walletAddress: walletAddress.toUpperCase(), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify case-insensitive matching is appropriate for Solana addresses.
Solana addresses are base58 encoded strings that are inherently case-sensitive. Making the comparison case-insensitive might lead to incorrect matches or potential security issues, as different case variations could represent different addresses.
Please verify that case-insensitive matching is the correct approach for Solana addresses:
Additionally, check if there are any existing Solana addresses in the database that would be affected by this change:
🏁 Script executed:
#!/bin/bash
# Description: Check for existing Solana addresses and their case variations
# Expected: Identify any potential case conflicts
# Search for Solana-related configurations or address patterns
rg -i "solana" --type ts -A 5 -B 5
# Check for any existing address validation or formatting logic
ast-grep --pattern 'function $_($$$) {
$$$
address
$$$
}'Length of output: 132884
🌐 Web query:
Are Solana wallet addresses case-sensitive? What is the standard format for Solana addresses?
💡 Result:
Solana wallet addresses are case-sensitive. This means every character—including uppercase and lowercase letters—must match exactly; even a single incorrect case will result in a completely different address, potentially causing permanent loss of funds if used incorrectly[1][10][4][7]. This case sensitivity comes from Solana’s use of Base58 encoding, which preserves upper and lower case distinctions to accurately represent the underlying public key[1][3].
Standard Format for Solana Addresses:
- Length: Typically between 32 and 44 characters (most commonly 44)[5][2][6].
- Encoding: Base58 (includes numbers and upper/lowercase letters, but excludes similar-looking characters: 0 (zero), O (uppercase o), I (uppercase i), and l (lowercase L))[1][3][5].
- Structure: Solana addresses do not have a fixed prefix (such as "0x" for Ethereum); they can start with any permitted Base58 character[6]. Some sources mention vanity addresses starting with an "S," but this is not a universal requirement—many valid Solana addresses begin with other characters[3][8][5].
Example of a valid Solana address:
HN7cABqLq46Es1jh92dQQisAq662SmxELLLsHHe4YWrH
or
7EcDhSYGxXyscszYEp35KHN8vvw3svAuLKTzXwCFLtV
[2][5]
Key points to remember:
- Always copy and paste the address rather than typing, to avoid errors with case sensitivity[1][10].
- Double-check the full address before sending funds, as funds sent to the wrong address due to case error are irretrievable on the Solana network[7][10].
- Solana also supports vanity (custom) addresses, which follow the same Base58 case-sensitive standard[8].
In summary:
Solana wallet addresses are case-sensitive, formatted as 32–44 character Base58-encoded strings that may include both upper- and lowercase letters and numbers (excluding 0, O, I, l), and do not have a standard prefix[1][5][6].
Citations:
- 1: https://academy.swissborg.com/en/learn/solana-addresses-case-sensitive
- 2: https://coin98.net/solana-wallet
- 3: https://www.bydfi.com/en/questions/are-there-any-specific-requirements-for-formatting-a-solana-address
- 4: https://academy.saakuru.com/which-blockchain-wallet-address-is-case-sensitive/
- 5: https://coin.space/solana-address-example/
- 6: https://cryptocurrencyhelp.com/article/whats-a-crypto-address-how-to-make-sense-of-all-the-numbers
- 7: https://support.atomicwallet.io/article/329-case-sensitivity-of-crypto-addresses
- 8: https://blog.slerf.tools/en-us/custom-solana-wallet-address-a-guide-to-vanity-address-generation-with-slerftools/
- 9: https://forum.solana.com/t/srfc-00007-encryption-standard-for-solana-keypairs/65
- 10: https://support.ledger.com/article/360016265659-zd
Enforce case-sensitive matching for Solana addresses
Solana addresses use Base58 encoding and are strictly case-sensitive. Using UPPER() and comparing on uppercase values will erroneously treat distinct addresses as identical and risks incorrect lookups or security issues.
Please update in src/repositories/projectAddressRepository.ts (inside findRelatedAddressByWalletAddress):
• Replace lines 49–50
- query = query.where(`UPPER(address) = :walletAddress`, {
- walletAddress: walletAddress.toUpperCase(),
- });
+ query = query.where(`address = :walletAddress`, {
+ walletAddress,
+ });This ensures an exact, case-sensitive match for Solana wallets.
📝 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.
| query = query.where(`UPPER(address) = :walletAddress`, { | |
| walletAddress: walletAddress.toUpperCase(), | |
| query = query.where(`address = :walletAddress`, { | |
| walletAddress, | |
| }); |
🤖 Prompt for AI Agents
In src/repositories/projectAddressRepository.ts around lines 49 to 50, the query
uses UPPER() to perform a case-insensitive match on Solana addresses, which are
case-sensitive. Remove the UPPER() function and the toUpperCase() call on
walletAddress, and update the query to compare the address field directly with
the walletAddress parameter to enforce exact, case-sensitive matching.
|
|
||
| export const validateProjectTitle = async (title: string): Promise<boolean> => { | ||
| const isTitleValid = /^[a-zA-Z0-9?!@#$%^&*+=._|/<">`'-]+$/.test( | ||
| const isTitleValid = /^[a-zA-Z0-9?!@#$%^&*+=._|/<">`'()[\]{},-]+$/.test( |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Approve the expanded character set for project titles.
The regex syntax is correct and the addition of parentheses (), square brackets [], curly braces {}, and comma , will improve user experience by allowing more expressive project titles.
However, please verify:
- That the
titleWithoutSpecialCharactersutility function (used ingetSimilarTitleInProjectsRegex) properly handles these new characters - That existing tests cover validation with these new characters
🏁 Script executed:
#!/bin/bash
# Description: Verify consistent handling of the new special characters across validation functions
# Test 1: Check if titleWithoutSpecialCharacters utility handles the new characters
echo "Checking titleWithoutSpecialCharacters utility function..."
ast-grep --pattern 'export const titleWithoutSpecialCharacters = $$$'
# Test 2: Look for existing tests for validateProjectTitle
echo "Checking for existing tests..."
rg -A 10 -B 5 "validateProjectTitle" --type ts
# Test 3: Check for any other project title validation logic
echo "Checking for other title validation patterns..."
rg -A 5 -B 5 "title.*regex|regex.*title" --type tsLength of output: 18071
Review expanded allowed characters – update utility & tests
The updated regex in validateProjectTitle correctly permits ()[]{} , -, but downstream logic and tests need to align:
-
In
src/utils/utils.ts(titleWithoutSpecialCharacters):
• It currently strips only the previous set of chars; it does not remove()[]{},-.
• To avoid unescaped regex metacharacters ingetSimilarTitleInProjectsRegex, either add these toUNALLOWED_SPECIAL_CHARACTERS_FOR_PROJECT_TITLEor escape them when constructing the RegExp from user input. -
In
src/utils/validators/projectValidator.test.ts:
• Remove or update the test that expectsfdf()to be invalid (parentheses are now allowed).
• Add positive test cases for titles containing each of the new chars:
– Parentheses()
– Square brackets[]
– Curly braces{}
– Comma,
– Dash- -
Verify no other title-comparison logic assumes the old restricted character set.
🤖 Prompt for AI Agents
In src/utils/validators/projectValidator.ts at line 106, the regex now allows
additional characters ()[]{},- but related code and tests are not updated.
Update src/utils/utils.ts in titleWithoutSpecialCharacters to also remove these
new allowed characters or add them to
UNALLOWED_SPECIAL_CHARACTERS_FOR_PROJECT_TITLE to ensure proper escaping in
getSimilarTitleInProjectsRegex. In
src/utils/validators/projectValidator.test.ts, remove or update the test that
marks titles like "fdf()" invalid and add new positive tests for titles
containing each of the new allowed characters: parentheses, square brackets,
curly braces, comma, and dash. Finally, review any other title comparison logic
to ensure it aligns with the expanded allowed character set.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
test/pre-test-scripts.ts (2)
563-564: Global flag approach still has race condition risks in parallel execution.The global flag implementation remains problematic for the parallel test execution strategy mentioned in the PR objectives, as multiple test processes could interfere with each other's initialization state.
719-724: Test gating mechanism may not be sufficient for parallel execution.While the
beforeEachhook provides a safety check, it may not handle scenarios where database initialization is in progress in another parallel process, as mentioned in previous reviews.
🧹 Nitpick comments (1)
test/pre-test-scripts.ts (1)
620-717: Enhanced test setup with improved logging and timeout handling.The restructured
beforehook provides excellent visibility into the setup process with detailed logging at each step. The 60-second timeout and explicit entity DataSource configuration address common initialization issues.However, there's a static analysis issue that should be addressed:
- if (dataSource && dataSource.isInitialized) { + if (dataSource?.isInitialized) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/pre-test-scripts.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
test/pre-test-scripts.ts
[error] 634-634: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
test/pre-test-scripts.ts (1)
570-617: Database verification function looks solid with comprehensive entity setup.The
ensureDatabaseReadyfunction provides thorough database connection verification and entity DataSource setup. The error handling and logging are appropriate for debugging initialization issues.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/pre-test-scripts.ts (2)
564-566: Race condition risk with global flags in parallel test executionThe global flag approach may cause issues when tests run in parallel, as multiple test processes could interfere with each other's initialization state.
This is the same concern raised in the previous review about potential race conditions with the global flag approach.
827-832: Test gating logic may not work effectively in parallel executionWhile the
beforeEachhook provides a safety check, it may not be sufficient for the parallel test execution strategy mentioned in the PR objectives.This matches the previous review comment about the beforeEach hook not being sufficient for parallel test execution.
🧹 Nitpick comments (4)
test/setup-test-db.ts (1)
24-26: Use optional chaining for cleaner conditional check.The static analysis suggestion to use optional chaining is valid and would improve readability.
Apply this diff to simplify the conditional:
- if (dataSource && dataSource.isInitialized) { + if (dataSource?.isInitialized) {.github/workflows/staging-pipeline.yml (1)
120-120: Fix trailing whitespace.Remove trailing spaces to satisfy linting requirements.
Apply this diff to remove trailing spaces:
- run: npm run db:migrate:run:test + run: npm run db:migrate:run:test - +Also applies to: 148-148
test/pre-test-scripts.ts (2)
2-2: Minor comment style improvementThe comment formatting could be more consistent with the codebase style.
-// Using real blockchain providers for tests +// Using real blockchain providers for tests
619-623: Add cleanup logic to the after hookThe
afterhook currently only logs completion but doesn't perform any actual cleanup. Consider adding database connection cleanup or other necessary teardown operations.-after(function () { - // Cleanup after tests - logger.debug('Test cleanup complete'); -}); +after(async function () { + try { + // Close database connection + const dataSource = AppDataSource.getDataSource(); + if (dataSource && dataSource.isInitialized) { + await dataSource.destroy(); + logger.debug('Database connection closed'); + } + } catch (error) { + logger.warn('Error during test cleanup', error); + } + logger.debug('Test cleanup complete'); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.cursor/rules/documentation-generation.mdc(1 hunks).github/workflows/develop-pipeline.yml(3 hunks).github/workflows/staging-pipeline.yml(4 hunks)package.json(2 hunks)src/server/bootstrap.ts(2 hunks)test/pre-test-scripts.ts(3 hunks)test/setup-test-db.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/server/bootstrap.ts (1)
src/utils/logger.ts (1)
logger(44-44)
🪛 Biome (1.9.4)
test/setup-test-db.ts
[error] 24-24: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
test/pre-test-scripts.ts
[error] 731-731: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 actionlint (1.7.7)
.github/workflows/develop-pipeline.yml
129-129: the runner of "actions/checkout@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
131-131: the runner of "actions/setup-node@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/staging-pipeline.yml
197-197: the runner of "actions/setup-node@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.37.1)
.github/workflows/staging-pipeline.yml
[error] 120-120: trailing spaces
(trailing-spaces)
[error] 148-148: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
src/server/bootstrap.ts (1)
4-4: LGTM! Dynamic port assignment enhances test isolation.The implementation correctly uses ephemeral ports (port 0) for test environments to avoid conflicts when running parallel tests, while maintaining the fixed port 4000 for other environments. The dynamic port retrieval ensures accurate logging.
Also applies to: 305-312
test/setup-test-db.ts (2)
12-57: Excellent test setup strategy for performance optimization.The database snapshotting approach is well-implemented and aligns perfectly with the PR objective of optimizing test execution time. The script properly handles bootstrap, database seeding, and snapshot creation with appropriate error handling and logging.
73-89: Good use of advisory locks for snapshot creation.The advisory lock mechanism ensures safe concurrent access during snapshot creation, which is crucial for parallel test execution.
.cursor/rules/documentation-generation.mdc (1)
1-49: Well-structured documentation guidelines.The documentation rule provides comprehensive guidelines for maintaining clear and consistent README files across the repository, which supports good development practices.
.github/workflows/staging-pipeline.yml (1)
38-193: Excellent workflow restructuring for test optimization.The separation of setup and test jobs with matrix strategy effectively implements the PR objective of parallelizing tests while maintaining proper database preparation. The test grouping strategy will significantly reduce overall pipeline execution time.
.github/workflows/develop-pipeline.yml (1)
12-127: Excellent parallel test execution strategy.The workflow restructuring effectively implements the PR objective by separating test setup from execution and using matrix strategy to run tests in parallel groups. This approach will significantly reduce CI/CD pipeline execution time.
test/pre-test-scripts.ts (6)
572-617: Database readiness verification function looks solidThe
ensureDatabaseReadyfunction properly verifies database connection, initializes DataSource if needed, and sets DataSource for all entities with appropriate error handling.
629-662: Database snapshot creation implementation is well-structuredThe
createDatabaseSnapshotfunction uses PostgreSQL advisory locks properly to ensure consistency and handles errors appropriately. The approach of copying all tables to a snapshot schema is efficient.
668-714: Database snapshot restoration logic is soundThe
restoreDatabaseFromSnapshotfunction properly truncates public tables (excluding migrations) and restores data from the snapshot schema. The error handling and logging are appropriate.
717-825: Enhanced before hook with comprehensive database setupThe
beforehook now includes proper database initialization, entity DataSource setup, and snapshot management. The logic for trying to restore from snapshot first, then falling back to seeding, is well-implemented.
834-842: Redis cleanup in afterEach hook is appropriateThe
afterEachhook properly clears Redis cache between tests to prevent interference, with appropriate error handling.
844-851: Export statements enable external usageThe exported functions allow the setup script and other components to reuse the database initialization and snapshot management logic.
package.json (1)
117-136: Tests already ensure isolation and run serially, no changes needed
- The
pre-test-scripts.tsfile seeds, migrates, creates a snapshot on first load, and restores that snapshot in a root‐levelbeforeEachhook—so every test (and every test file) starts from a clean database state.- None of the scripts invoke Mocha’s
--parallelflag or spawn multiple worker processes; the “parallel” grouping simply runs categories one after another, so there’s no risk of CI resource exhaustion from uncontrolled concurrency.No actionable refactors are required.
| uses: actions/setup-node@v1 | ||
| with: | ||
| node-version: 20.11.0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update deprecated GitHub Action version.
The actions/setup-node@v1 action is deprecated and should be updated to avoid potential compatibility issues.
Apply this diff to update the action version:
- uses: actions/setup-node@v1
+ uses: actions/setup-node@v4📝 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.
| uses: actions/setup-node@v1 | |
| with: | |
| node-version: 20.11.0 | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version: 20.11.0 |
🧰 Tools
🪛 actionlint (1.7.7)
197-197: the runner of "actions/setup-node@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/staging-pipeline.yml around lines 197 to 199, the GitHub
Action 'actions/setup-node@v1' is deprecated. Update the action version to the
latest stable release, such as 'actions/setup-node@v3', by changing the version
tag in the uses field to ensure compatibility and support.
| 'migrations' | ||
| ] | ||
| steps: | ||
| - uses: actions/checkout@v1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update deprecated GitHub Action versions.
The actions/checkout@v1 and actions/setup-node@v1 actions are deprecated and should be updated to avoid potential compatibility issues.
Apply this diff to update the action versions:
- - uses: actions/checkout@v1
+ - uses: actions/checkout@v4
- name: Use Node.js
- uses: actions/setup-node@v1
+ uses: actions/setup-node@v4Also applies to: 131-131
🧰 Tools
🪛 actionlint (1.7.7)
129-129: the runner of "actions/checkout@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/develop-pipeline.yml at lines 129 and 131, the GitHub
Actions 'actions/checkout@v1' and 'actions/setup-node@v1' are deprecated. Update
these action versions to the latest stable releases, such as
'actions/checkout@v3' and 'actions/setup-node@v3', to ensure compatibility and
avoid potential issues.
divine-comedian
force-pushed
the
test-refactor
branch
from
9a73449 to
57c8ff7
Compare
|
@divine-comedian, have you given up on this? thx |
2 participants
attempts to parralelize some tests and preserve other tests to run sequentially, potentially reducing automated test run time.
Summary by CodeRabbit
Documentation
Chores
Bug Fixes