-
Notifications
You must be signed in to change notification settings - Fork 55
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
chore: add tests for paymaster contracts using era-test-node-action #15
Conversation
|
||
- name: Run Tests | ||
run: | | ||
yarn ci:tests |
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.
suggestion: We should consider always printing out the logs of era_test_node
in case the tests fail
- name: Era Test Node Logs
if: always()
run: cat ./era_test_node.log
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 think if a job fails it would be best the user updates the action to use logs:debug and then make use of the log file (either cat
or artifacts approach) which I don't think should be used by default?
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.
Up to you really, but I'd also prefer the CI showing test logs by default
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 suggestions.
|
||
- name: Run Tests | ||
run: | | ||
yarn ci:tests |
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.
Up to you really, but I'd also prefer the CI showing test logs by default
@@ -0,0 +1,27 @@ | |||
name: run |
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.
Your steps have such nice capitalised titles and this one's just called "run" 😆
const zkSyncTestnet = | ||
process.env.NODE_ENV == "test" | ||
? { | ||
const getNetworkConfig = () => { |
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 like this, we should use this in other examples too come to think of it
Changes introduced in this PR: