-
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
feat: add workflow which allows to start tests against desired tag #649
feat: add workflow which allows to start tests against desired tag #649
Conversation
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Test Results242 tests ±0 236 ✔️ ±0 7m 42s ⏱️ +18s Results for commit a4734bd. ± Comparison against base commit 8cabc7f. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: Yaroslav Markovski <[email protected]>
68af2e9
to
0cd5e57
Compare
Signed-off-by: georgi-l95 <[email protected]>
@@ -47,7 +50,7 @@ jobs: | |||
version: nightly | |||
|
|||
- name: Start the local node | |||
run: npx hedera start -d --network local | |||
run: npx hedera start -d --network-tag=${{inputs.networkTag}} --mirror-tag=${{inputs.mirrorTag}} --relay-tag=${{inputs.relayTag}} --verbose=trace |
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.
Question: Above I saw the networkTag, mirrorTag, relayTag are marked required:false, does that mean if those tags are not entered, local node will pick up the latest tags?
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.
yep, local node will start with local configuration(which is usually latest tags)
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.
Nice work! Will be back for a full review soon
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.
My idea with this manual workflow is to be triggered, when for example we have a new network tag. Instead of checking out repos locally, starting local-node and running tests, we can do it here with a simple click.
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.
Nice initiative!
testfilter: PrngSystemContract | ||
networkTag: ${{inputs.networkNodeTag}} | ||
mirrorTag: ${{inputs.mirrorNodeTag}} | ||
relayTag: ${{inputs.relayTag}} |
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.
Do you think we should make some room for the Solidity, Yul, and OZ contracts as well? https://github.com/hashgraph/hedera-smart-contracts/blob/main/.github/workflows/solidity-tests.yml
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.
Might be too much for this case but not a bad idea.
@georgi-l95 have you tried the run out?
How long does everything take already and then when you add the above suggestions?
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.
Yep, we can add them, they run in parallel, so it shouldn't run longer than now, which is as long as the slowest test (around 5 minutes)
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.
Nice addition.
A few questions
testfilter: PrngSystemContract | ||
networkTag: ${{inputs.networkNodeTag}} | ||
mirrorTag: ${{inputs.mirrorNodeTag}} | ||
relayTag: ${{inputs.relayTag}} |
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.
Might be too much for this case but not a bad idea.
@georgi-l95 have you tried the run out?
How long does everything take already and then when you add the above suggestions?
Signed-off-by: georgi-l95 <[email protected]>
d148b17
to
d0d1ab2
Compare
@@ -324,16 +324,16 @@ describe('Multicall Test Suite', function () { | |||
expect(receipt.status).to.eq(1); | |||
}); | |||
|
|||
it('should NOT be able to aggregate 150 calls to processLongInputTx', async function () { | |||
const n = 150; | |||
it('should NOT be able to aggregate 200 calls to processLongInputTx', async function () { |
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.
this change is needed, because of this PR in the relay, which changes chunk size to around 50kb
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.
Oh is that what it is? No wonder it kept failing in the ci
Signed-off-by: georgi-l95 <[email protected]>
c9b0480
to
a4734bd
Compare
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. Thanks for the work!
@@ -324,16 +324,16 @@ describe('Multicall Test Suite', function () { | |||
expect(receipt.status).to.eq(1); | |||
}); | |||
|
|||
it('should NOT be able to aggregate 150 calls to processLongInputTx', async function () { | |||
const n = 150; | |||
it('should NOT be able to aggregate 200 calls to processLongInputTx', async function () { |
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.
Oh is that what it is? No wonder it kept failing in the ci
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.
That's a great addition!
Description:
This PR aims to add manual workflow, which can be triggered by specifying tag which the tests should run against.
Related issue(s):
Fixes #648
Notes for reviewer:
Checklist