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.

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

fix: fix E2E test failures in Ink contracts #1876

Conversation

0xf333
Copy link

@0xf333 0xf333 commented Aug 14, 2023

Description

The e2e-tests for ink contracts were failing due to an issue with port parsing. This PR introduces:

  • tokio::task::spawn_blocking to offload the task to a separate thread.
  • std::sync::mpsc channel to efficiently relay the extracted port information.
  • Continuous reading of process output to prevent any potential blockages.


What This Enables

You can now reliably execute e2e-tests for ink contracts.

Impacts

This pull request addresses issue #1875



Testing The Implemented Fix

Environment ==> **Linux**

  1. Clone this repo:
git clone https://github.com/0xf333/scenario_for_E2E_test_issue
  1. Navigate to /scenario_for_E2E_test_issue/test_PR_fix and make substrate-contracts-node and
    env_setup.sh executable
chmod +x substrate-contracts-node env_setup.sh
  1. Navigate to /scenario_for_E2E_test_issue/test_PR_fix/flipper:
cd flipper
  1. Compile the contract; you need to do this to pull all the necessary dependencies.
    (Wait for the compilation to finish before proceeding to the next step.)
cargo contract build --release
  1. Navigate back to the test_PR_fix directory:
cd ..
  1. Run the env_setup.sh bash script to:
    • Set the CONTRACTS_NODE environment variable.
    • Get the path to the crate file where you need to apply this PR fix.
    • Get the path string required to accordingly set ink_e2e inside
      the Cargo.toml file in order to test this PR fix.
source env_setup.sh

Note: Make sure to use source instead of ./ to ensure the CONTRACTS_NODE environment variable is properly set.

  1. Copy and paste the changes from this PR into the .rs file ( you'll get the file path after running the bash script ).
    Save the change and then go back to the flipper directory, and run these commands in the same order:
cargo clean
cargo cargo contract build --release
cargo test --features e2e-tests

clean to clean the previous build
conrtact build --release to compile the contract and include changes made inside the ink_e2e dependency.
test --features e2e-tests to run the E2E test

Finally, after executing cargo test --features e2e-tests, make sure that the compilation completes entirely. If you encounter an interruption at:

 [2/4] Post processing wasm file
error: custom attribute panicked

Run the command again. Occasionally in Rust, compilation may prematurely exit during heavy CPU load.



Result

Before the fix

before.mp4

After the fix

after.mp4

@0xf333 0xf333 marked this pull request as draft August 14, 2023 12:10
@0xf333
Copy link
Author

0xf333 commented Aug 14, 2023

meant to be a draft, not a PR yet.

@0xf333 0xf333 force-pushed the 0xf333/fix_E2E_test_issue_for_ink_contracts branch 9 times, most recently from 5454a92 to 79debad Compare August 14, 2023 14:55
This commit addresses the issue where e2e-tests for ink contracts
are failing even with the correct substrate-contracts-node setup.

Change made
===========
- I've Introduced `tokio::task::spawn_blocking` to offload the
  task of reading the substrate process's output to a separate
  thread since the previous approach wasn't working.
- I've used `std::sync::mpsc` channel to relay the extracted port
  from the reader thread back to the main async context.
- I've improved the `find_substrate_port_from_output` function to
  ensure continuous reading of process output, in order to prevent
  potential blockages.

Note:
=====
For details on how to test this fix, please read the PR write up.

Impact:
=======
This commit addresses issue use-ink#1875
@0xf333 0xf333 force-pushed the 0xf333/fix_E2E_test_issue_for_ink_contracts branch from 79debad to 557c314 Compare August 14, 2023 15:02
@0xf333 0xf333 marked this pull request as ready for review August 14, 2023 15:40
@0xf333
Copy link
Author

0xf333 commented Aug 14, 2023

Hi guys, it's now ready for review.

Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

Looking at your reproduction, in the issue/flipper/Cargo.toml you are using the 4.2.1 released version of ink_e2e on crates.io. This is not compatible with the latest substrate-contracts-node release.

I would bet that if you change that to use ink master then it will resolve the issue without these changes.

Our CI ensures that the master branch can run all E2E tests against the latest substrate-contracts-node

@0xf333
Copy link
Author

0xf333 commented Aug 15, 2023

Looking at your reproduction, in the issue/flipper/Cargo.toml you are using the 4.2.1 released version of ink_e2e on crates.io. This is not compatible with the latest substrate-contracts-node release.

I would bet that if you change that to use ink master then it will resolve the issue without these changes.

Our CI ensures that the master branch can run all E2E tests against the latest substrate-contracts-node

Hi @ascjones,
Thanks for the feedback, this case is present in ink_e2e 4.2.0 too; the reason I used 4.2.1 is that I thought it would make more sense to solve it in the latest version of ink_e2e rather than in an older one.
Please let me know what you think.

@0xf333 0xf333 requested a review from ascjones August 15, 2023 15:21
@ascjones
Copy link
Collaborator

ascjones commented Aug 16, 2023

Looking at your reproduction, in the issue/flipper/Cargo.toml you are using the 4.2.1 released version of ink_e2e on crates.io. This is not compatible with the latest substrate-contracts-node release.
I would bet that if you change that to use ink master then it will resolve the issue without these changes.
Our CI ensures that the master branch can run all E2E tests against the latest substrate-contracts-node

Hi @ascjones, Thanks for the feedback, this case is present in ink_e2e 4.2.0 too; the reason I used 4.2.1 is that I thought it would make more sense to solve it in the latest version of ink_e2e rather than in an older one. Please let me know what you think.

None of the crates.io releases 4.2.0, 4.2.1, or any others are compatible with the latest substrate-contracts-node, because they were released before the node was updated. They will only work with an older version of substrate-contracts-node, and have already been released so can't be changed.

The ink! master branch is already compatible with the latest substrate-contracts-node: you can prove it by navigating to any integration_tests example and running the e2e tests (this is also checked by a CI step).

If you want to use e2e tests with any of the released ink_e2e versions, then use an older release of substrate-contracts-node

If you want to use the latest substrate-contracts-node (which you are doing) then use the ink master branch via a git dependency.

Thank you for the detailed report and reproduction steps, but I don't think these changes are required.

@ascjones ascjones closed this Aug 16, 2023
@0xf333
Copy link
Author

0xf333 commented Aug 16, 2023

None of the crates.io releases 4.2.0, 4.2.1, or any others are compatible with the latest substrate-contracts-node, because they were released before the node was updated. They will only work with an older version of substrate-contracts-node, and have already been released so can't be changed.

The ink! master branch is already compatible with the latest substrate-contracts-node: you can prove it by navigating to any integration_tests example and running the e2e tests (this is also checked by a CI step).

If you want to use e2e tests with any of the released ink_e2e versions, then use an older release of substrate-contracts-node

If you want to use the latest substrate-contracts-node (which you are doing) then use the ink master branch via a git dependency.

Thank you for the detailed report and reproduction steps, but I don't think these changes are required.

Hi @ascjones,
Thanks for the feedback.

@ascjones
Copy link
Collaborator

@0xf333 we are considering a 4.x release with a backport to fix this issue. Stay tuned.

@0xf333
Copy link
Author

0xf333 commented Aug 18, 2023

@0xf333 we are considering a 4.x release with a backport to fix this issue. Stay tuned.

Hi @ascjones , I sent you an email, please check and let me know.

@ascjones
Copy link
Collaborator

See #1884 for backport of fix, will do another 4.x.x series release soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants