Skip to content

Update Deterministic hardhat via viatrix PR#164

Closed
LyonSsS wants to merge 29 commits intomainfrom
feat/update_deterministic_hardhat_tests
Closed

Update Deterministic hardhat via viatrix PR#164
LyonSsS wants to merge 29 commits intomainfrom
feat/update_deterministic_hardhat_tests

Conversation

@LyonSsS
Copy link
Copy Markdown
Contributor

@LyonSsS LyonSsS commented Dec 17, 2025

Added hardcoded Blocks for forked blockchain to keep it constant locally and on GitHub CI action

@LyonSsS LyonSsS requested a review from lastperson December 17, 2025 12:07
@LyonSsS LyonSsS requested a review from viatrix December 18, 2025 09:50
viatrix
viatrix previously approved these changes Dec 18, 2025
echo "=================================================="

# Parse CI-generated coverage using dedicated script
CI_LINES=$(npx ts-node --files scripts/get-coverage-percentage.ts)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is the get-coverage-percentage script not used anymore, and instead inline nodejs code is used?

Comment on lines -82 to -83
PR_LINES=$(jq -r .lines baseline-from-pr.json)
MAIN_LINES=$(jq -r .lines baseline-from-main.json)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why abandoning this part if it was perfectly alright?

# Fetch main branch
git fetch origin main
# Get baseline from main (for comparison - must not decrease)
git show origin/main:coverage-baseline.json > baseline-from-main.json 2>/dev/null || echo '{"lines":"0","functions":"0","branches":"0","statements":"0"}' > baseline-from-main.json
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why this code was abandoned?

Comment thread hardhat.config.ts Outdated
Comment on lines +701 to +702
: (process.env.FORK_PROVIDER || process.env.BASE_RPC || "https://base-mainnet.public.blastapi.io"),
blockNumber: process.env.FORK_BLOCK_NUMBER ? parseInt(process.env.FORK_BLOCK_NUMBER) : undefined,
: (process.env.BASE_RPC || "https://base-mainnet.public.blastapi.io"),
blockNumber: undefined,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why changing this logic? Is there a problem with it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was not the case for the discrepancy, so forked block creates more complexity as we need to update it from time to time. I opt again for "latest"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is the diff with main branch, before the introduction of coverage test on a particular block.
FORK_PROVIDER and FORK_BLOCK_NUMBER logic is used for other purposes as well.

Comment thread scripts/check-coverage.ts Outdated
Comment on lines +47 to +50
lines: linesFound > 0 ? ((linesHit / linesFound) * 100).toFixed(2) : "0",
functions: functionsFound > 0 ? ((functionsHit / functionsFound) * 100).toFixed(2) : "0",
branches: branchesFound > 0 ? ((branchesHit / branchesFound) * 100).toFixed(2) : "0",
statements: linesFound > 0 ? ((linesHit / linesFound) * 100).toFixed(2) : "0",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All new parenthesis don't change the results, so why adding them?

Comment thread scripts/check-coverage.ts
// Tolerant comparison
function checkDrop(metric: keyof CoverageData, baseline: CoverageData, label: string): string | null {
const base = parseFloat(baseline[metric]);
const curr = parseFloat(current[metric]);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

current should be in function arguments instead of using a global one.

@lastperson
Copy link
Copy Markdown
Collaborator

Added tolerance in the arbitrum native bridge branch.

@lastperson lastperson closed this Mar 24, 2026
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.

3 participants