Skip to content

chore(idea/vara-eth): setup explorer tests#2502

Open
osipov-mit wants to merge 3 commits into
mainfrom
do-vara-eth-explorer-tests
Open

chore(idea/vara-eth): setup explorer tests#2502
osipov-mit wants to merge 3 commits into
mainfrom
do-vara-eth-explorer-tests

Conversation

@osipov-mit
Copy link
Copy Markdown
Member

No description provided.

@osipov-mit osipov-mit requested a review from vraja-nayaka May 26, 2026 12:25
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a comprehensive suite of Vitest integration tests, a database dump generation script, and a corresponding SQL dump for the explorer module. It also updates the QueryStateTransitionsDto to use a custom @Transform decorator for parsing the exited boolean query parameter. The review feedback highlights several critical issues: a bug in the @Transform logic that silently swallows invalid query parameters instead of throwing a validation error, a runtime TypeError in the tests due to using a non-standard matcher (toBeOneOf), potential cross-platform failures when parsing line endings in the SQL dump, and a lack of cleanup traps in the shell script if it exits prematurely.

Comment on lines +31 to +34
@Transform(({ value }) => {
if (value === undefined || value === null) return undefined;
return value === true || value === 'true';
})
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The current implementation of the @Transform decorator converts any value that is not true or 'true' to false. This means that invalid query parameters (such as exited=invalid) will be silently transformed to false and pass @IsBoolean() validation instead of triggering a 400 Bad Request error. To preserve validation of invalid inputs, only transform valid boolean representations and return the original value otherwise.

Suggested change
@Transform(({ value }) => {
if (value === undefined || value === null) return undefined;
return value === true || value === 'true';
})
@Transform(({ value }) => {
if (value === 'true' || value === true) return true;
if (value === 'false' || value === false) return false;
return value;
})

expect(res.status).toBe(200);
expect(res.body.data.length).toBe(3);
res.body.data.forEach((p: any) => {
expect(p.id).toBeOneOf([PROGRAM_1_ID, PROGRAM_2_ID, PROGRAM_3_ID]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The toBeOneOf matcher is not a standard Vitest/Jest matcher and is typically provided by jest-extended. Since jest-extended is not configured or imported in the test setup, this assertion will throw a runtime TypeError. Use the standard toContain matcher instead.

Suggested change
expect(p.id).toBeOneOf([PROGRAM_1_ID, PROGRAM_2_ID, PROGRAM_3_ID]);
expect([PROGRAM_1_ID, PROGRAM_2_ID, PROGRAM_3_ID]).toContain(p.id);


const dump = readFileSync(DUMP_PATH, 'utf8');
const lines = dump.split('\n');
const out: string[] = [];
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Splitting the dump file by \\n can cause issues on Windows environments or when git checkouts use CRLF line endings, as the trailing \\r will remain on each line. This will cause strict string comparisons like line === '\\\\.' to fail. Use a regular expression to split by both \\r\\n and \\n to ensure cross-platform compatibility.

Suggested change
const out: string[] = [];
const lines = dump.split(/\\r?\\n/);

Comment on lines +58 to +61
printf 'Creating temporary database "%s"...\n' "$TMP_DB"
if [ "$(db_exists "$TMP_DB")" != "1" ]; then
psql -h "$DB_HOST" -p "$DB_PORT" -U "$DB_USER" -d postgres -c "CREATE DATABASE \"$TMP_DB\""
fi
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

If the script fails or exits prematurely (which is likely due to set -e), the temporary database created for the dump will not be dropped, leaving dangling databases. Using an EXIT trap ensures that the temporary database is always cleaned up regardless of whether the script succeeds or fails.

Suggested change
printf 'Creating temporary database "%s"...\n' "$TMP_DB"
if [ "$(db_exists "$TMP_DB")" != "1" ]; then
psql -h "$DB_HOST" -p "$DB_PORT" -U "$DB_USER" -d postgres -c "CREATE DATABASE \"$TMP_DB\""
fi
printf 'Creating temporary database "%s"...\\n' "$TMP_DB"
if [ "$(db_exists "$TMP_DB")" != "1" ]; then
psql -h "$DB_HOST" -p "$DB_PORT" -U "$DB_USER" -d postgres -c "CREATE DATABASE \"$TMP_DB\""
fi
cleanup() {
printf 'Dropping temporary database "%s"...\\n' "$TMP_DB"
psql -h "$DB_HOST" -p "$DB_PORT" -U "$DB_USER" -d postgres -c "DROP DATABASE IF EXISTS \"$TMP_DB\"" >/dev/null 2>&1 || true
}
trap cleanup EXIT INT TERM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant