Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ci] #4151: Enhance the test environment by automatically generating WASM files for the smartcontracts to be tested #4187

Merged
merged 5 commits into from
Jan 22, 2024
Merged

[ci] #4151: Enhance the test environment by automatically generating WASM files for the smartcontracts to be tested #4187

merged 5 commits into from
Jan 22, 2024

Conversation

Asem-Abdelhady
Copy link
Contributor

@Asem-Abdelhady Asem-Abdelhady commented Jan 9, 2024 •
edited
Loading

Description

I made a shell script to create test-smartcontracts directory and build the WASM files in that directory

Linked issue

Closes #4151

Benefits

No need to manually generate the WASM files.

Checklist

  • I've read CONTRIBUTING.md
  • I've used the standard signed-off commit format (or will squash just before merging)
  • All applicable CI checks pass (or I promised to make them pass later)
  • I replied to all comments after code review, marking all implemented changes with thumbs up

@Asem-Abdelhady Asem-Abdelhady changed the title [CI] #4151: Enhance the test environment by automatically generating WASM files for the smartcontracts to be tested [ci] #4151: Enhance the test environment by automatically generating WASM files for the smartcontracts to be tested Jan 9, 2024
@Erigara Erigara added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Jan 9, 2024
@Erigara
Copy link
Contributor

Erigara commented Jan 9, 2024

Please only add relevant commits

@Asem-Abdelhady Asem-Abdelhady force-pushed the test_env_improvment branch 2 times, most recently from 746b3a3 to 395591a Compare January 9, 2024 15:00
Erigara
Erigara reviewed Jan 9, 2024
View reviewed changes
scripts/test_env.py Outdated Show resolved Hide resolved
scripts/test_env.py Outdated Show resolved Hide resolved
Erigara
Erigara reviewed Jan 10, 2024
View reviewed changes
scripts/test_env.py Outdated Show resolved Hide resolved
Erigara
Erigara reviewed Jan 10, 2024
View reviewed changes
Copy link
Contributor

@Erigara Erigara left a comment

Choose a reason for hiding this comment

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

I was thinking about should we make smartcontract build optional?

@Asem-Abdelhady
Copy link
Contributor Author

I was thinking about should we make smartcontract build optional?

@Erigara We can add argument --wasm_generation with default true value. We check the arg before the generation step.

@Erigara
Copy link
Contributor

Erigara commented Jan 10, 2024

I suggested to make this optional is the fact that this functionality is only necessary to run python integration tests.
So if i don't need wasm files i could skip this step and don't waste my time.

mversic
mversic reviewed Jan 11, 2024
View reviewed changes
scripts/test_env.py Outdated Show resolved Hide resolved
@mversic
Copy link
Contributor

mversic commented Jan 11, 2024

I suggested to make this optional is the fact that this functionality is only necessary to run python integration tests.
So if i don't need wasm files i could skip this step and don't waste my time.

I don't think we should put the test smart contract build into test_env script at all

@Asem-Abdelhady
Copy link
Contributor Author

I suggested to make this optional is the fact that this functionality is only necessary to run python integration tests.
So if i don't need wasm files i could skip this step and don't waste my time.

I don't think we should put the test smart contract build into test_env script at all

Do you think it is better to add it in the CI process? or not to do it at all?

@Stukalov-A-M
Copy link
Contributor

@Asem-Abdelhady, the Iroha-java team is already created script for autogenerating WASMs, I think you may just reuse it.
https://github.com/hyperledger/iroha-java/blob/iroha2-dev/modules/client/src/test/executor/build.sh

@0x009922
Copy link
Contributor

@Stukalov-A-M, IMO both scripts are quite small and trivial, and it doesn't seem like there is something to reuse really.

@Asem-Abdelhady
Copy link
Contributor Author

@Stukalov-A-M, IMO both scripts are quite small and trivial, and it doesn't seem like there is something to reuse really.

i agree

0x009922
0x009922 reviewed Jan 18, 2024
View reviewed changes
.gitignore Show resolved Hide resolved
scripts/generate_wasm.sh Outdated Show resolved Hide resolved
Erigara
Erigara reviewed Jan 19, 2024
View reviewed changes
scripts/generate_wasm.sh Outdated Show resolved Hide resolved
AlexStroke
AlexStroke previously approved these changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@AlexStroke AlexStroke left a comment

Choose a reason for hiding this comment

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

LGTM

Erigara
Erigara previously approved these changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@Erigara Erigara left a comment

Choose a reason for hiding this comment

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

LGTM

0x009922
0x009922 previously approved these changes Jan 22, 2024
View reviewed changes
@Asem-Abdelhady Asem-Abdelhady dismissed stale reviews from 0x009922, Erigara, and AlexStroke via e26f7f0 January 22, 2024 09:24
@Asem-Abdelhady Asem-Abdelhady force-pushed the test_env_improvment branch 4 times, most recently from a1738d9 to 60b4f30 Compare January 22, 2024 09:47
@Asem-Abdelhady Asem-Abdelhady merged commit 57f87af into hyperledger-iroha:iroha2-dev Jan 22, 2024
1 check passed
@Asem-Abdelhady Asem-Abdelhady deleted the test_env_improvment branch January 23, 2024 07:38
Asem-Abdelhady added a commit to Asem-Abdelhady/iroha that referenced this pull request Feb 11, 2024
@Asem-Abdelhady
[ci] hyperledger-iroha#4151: Enhance the test environment by automati…
17cb5f3 
…cally generating WASM files for the smartcontracts to be tested (hyperledger-iroha#4187)

[add] generate_wasm.sh to generate WASM files of the smart contracts

Signed-off-by: Asem-Abdelhady <[email protected]>

---------

Signed-off-by: Asem-Abdelhady <[email protected]>
@alexstroke1 alexstroke1 assigned alexstroke1 and unassigned AlexStroke Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mversic mversic mversic left review comments

@0x009922 0x009922 0x009922 approved these changes

@Erigara Erigara Erigara approved these changes

@AlexStroke AlexStroke AlexStroke left review comments

@s8sato s8sato Awaiting requested review from s8sato

@Arjentix Arjentix Awaiting requested review from Arjentix

@SamHSmith SamHSmith Awaiting requested review from SamHSmith

@QuentinI QuentinI Awaiting requested review from QuentinI

@outoftardis outoftardis Awaiting requested review from outoftardis

@ilchu ilchu Awaiting requested review from ilchu

@pesterev pesterev Awaiting requested review from pesterev

@DCNick3 DCNick3 Awaiting requested review from DCNick3

Assignees

@Erigara Erigara

@Asem-Abdelhady Asem-Abdelhady

@alexstroke1 alexstroke1

Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Asem-Abdelhady @Erigara @mversic @AlexStroke @Stukalov-A-M @0x009922 @alexstroke1