Skip to content
This repository was archived by the owner on Feb 6, 2026. It is now read-only.

fix: integration test#96

Merged
suchapalaver merged 3 commits into
mainfrom
suchapalaver/fix/integration-test
Jun 12, 2025
Merged

fix: integration test#96
suchapalaver merged 3 commits into
mainfrom
suchapalaver/fix/integration-test

Conversation

@suchapalaver

@suchapalaver suchapalaver commented Jun 12, 2025

Copy link
Copy Markdown
Contributor

This PR fixes a hidden test bug discovered during local testing and prevents similar issues by adding integration tests to CI.

Fix Test Bug and Add Integration Testing to CI

🎯 Problem

While setting up local testing using the existing scripts/integration_tests.sh, we discovered:

  1. Hidden Test Bug: testSignerStillThawing() was failing with a timestamp mismatch - the test was incorrectly using block.number instead of block.timestamp
  2. No CI Coverage: Integration tests existed but weren't running in CI, so this bug went undetected
  3. Dependency Issues: The integration script needed improvements for Node.js version management

🔧 Solution

1. Fixed Test Bug

  • Discovery: Found during local integration testing that testSignerStillThawing() was failing
  • Root Cause: Test expected SignerStillThawing(uint256,uint256) error with (block.number, thawEndTimestamp)
  • Contract Reality: Error is thrown with (block.timestamp, thawEndTimestamp) - both are timestamps
  • Fix: Updated test to use block.timestamp to match actual contract behavior

2. Enhanced Integration Script Robustness

  • Node Version Management: Proper switching between Node 16 (Graph contracts) and Node 18 (TAP contracts)
  • Dependency Installation: Automatic yarn installation for each Node environment
  • Deployment Fixes: Added --broadcast flags and ETHERSCAN_API_KEY handling

3. Added Integration Tests to CI

  • Preventive Measure: Ensure integration bugs like this are caught automatically
  • New CI Job: integration-tests job runs after unit tests pass
  • Full Deployment Validation: Tests complete contract deployment flow
  • Environment Setup: Proper nvm, Node.js, and yarn installation

📝 Changes Made

Files Modified:

  • test/Escrow.t.sol - Fixed test to use correct timestamp parameter
  • scripts/integration_tests.sh - Enhanced Node version management and deployment robustness
  • .github/workflows/test.yml - Added integration test job to catch issues in CI

@suchapalaver suchapalaver force-pushed the suchapalaver/fix/integration-test branch from 2441f6a to 3c53bb4 Compare June 12, 2025 17:02
Signed-off-by: Joseph Livesey <joseph@semiotic.ai>
Signed-off-by: Joseph Livesey <joseph@semiotic.ai>
@suchapalaver suchapalaver force-pushed the suchapalaver/fix/integration-test branch from 3c53bb4 to a8a02f8 Compare June 12, 2025 17:03
Signed-off-by: Joseph Livesey <joseph@semiotic.ai>
@suchapalaver suchapalaver force-pushed the suchapalaver/fix/integration-test branch from a8a02f8 to 4011c6e Compare June 12, 2025 17:06
Comment thread test/Escrow.t.sol
abi.encodeWithSignature(
"SignerStillThawing(uint256,uint256)",
block.number,
block.timestamp,

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.

@tmigone I wanted to ask you about this change. Does this make sense? The script hadn't been part of CI and was written 2 years ago.

@suchapalaver suchapalaver merged commit e3351e7 into main Jun 12, 2025
7 checks passed
@suchapalaver

Copy link
Copy Markdown
Contributor Author

Thanks @tmigone!

@suchapalaver suchapalaver deleted the suchapalaver/fix/integration-test branch June 12, 2025 17:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants