-
Notifications
You must be signed in to change notification settings - Fork 22
Separate tests, fix linting, and utilize CI #43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
thodges-gh
commented
Sep 24, 2025
- Separates Hedera integration tests from Sepolia/Fuji integration tests
- Fixes unit tests and adds them to CI
- Updates linting and adds it as a step to CI
- No more blocks of skipped tests
👋 thodges-gh, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - minor comments
"t:int:ethers": "jest --coverage -u --testMatch=\"**/integration-testnet-ethers.test.ts\" --detectOpenHandles", | ||
"t:unit": "jest --coverage -u --testMatch=\"**/unit.test.ts\" ", | ||
"t:int": "jest --coverage -u --testMatch=\"**/integration-testnet**.test.ts\" --runInBand --detectOpenHandles --forceExit", | ||
"t:int:viem": "jest --coverage -u --testMatch=\"**/integration-testnet.test.ts\" --runInBand --detectOpenHandles --forceExit", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--forceExit is generally not recommended. If its the best way for now then perhaps update the comment I left in integration test file, line 22 or so that is marked with a TODO? just to explain the context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the comment to explain. I gave it a quick shot keep the tests from hanging without forceExit and wasn't successful. So probably best for that to be a chore for later.
const HEDERA_TESTNET_RPC_URL = process.env.HEDERA_TESTNET_RPC_URL || 'https://testnet.hashio.io/api' | ||
const SEPOLIA_CHAIN_SELECTOR = '16015286601757825753' | ||
|
||
describe('Integration: Hedera -> Sepolia', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Integration: Hedera -> Sepolia (viem)
? Make it explicitly viem only? and do we not want to test for ethers? Perhaps we can test both by using different clients in different it() blocks? Ugly but serves the same purpose as fulled duped tests without the duping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll mark it as viem for now and I want to revisit the tests between the providers in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM