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

Optimise CI - reduce flakyness, unify workflows #4516

Closed
0x009922 opened this issue Apr 26, 2024 · 5 comments · Fixed by #5124 · May be fixed by #5125
Closed

Optimise CI - reduce flakyness, unify workflows #4516

0x009922 opened this issue Apr 26, 2024 · 5 comments · Fixed by #5124 · May be fixed by #5125
Assignees
Labels
CI iroha2-dev The re-implementation of a BFT hyperledger in RUST question Further information is requested

Comments

@0x009922
Copy link
Contributor

I encounter this time and time again: you open a PR, wait for checks for 10/20/30 minutes, and then you see that same workflows fail on the same tests. During that time I get distracted on something else and forget to re-start checks again just in time. Sometimes it takes 5-6 restarts to get them work. This way, the moment of all green lights might delay for days. And it delays development in my case.

From my observation, the following workflows are flaky:

  • I2::Dev::Tests > with_coverage, integration, unstable

And these are particular flaky tests:

  • integration::extra_functional::offline_peers::genesis_block_is_committed_with_some_offline_peers
  • integration::extra_functional::unstable_network::soft_fork
  • And maybe some others, haven't collected much data

Are these tests worth it?

My another concern is that I don't see the rationale behind having so many workflows:

  1. I2::Dev::Static
    1. smart contracts
    2. workspace
  2. I2::Tests::UI
    1. test with all features
    2. test with no default features
  3. I2::Dev::Tests
    1. consistency
    2. with_coverage
    3. integration
    4. unstable
    5. client-cli-tests

(there are some others too)

These workflows all run Cargo and compile more or less the same stuff. Yes, there are variations in features presets, but Cargo handles it for us. It can granularly reuse compilation artifacts depending on the context (apart from cases with different RUSTC flags, I suppose).

So, I guess that it is worth trying to combine all these workflows into a single one, and build it in a way so that it can report as many useful information as possible in a single run. I wonder how much more/less it would be efficient.

Another useful implication of this would be a shorter feedback on some early errors. For example, a certain change in PR introduces something and Iroha cannot even compile. Currently, all 8+ workflows will run and fail on the same error. In the case of a unified CI, there will be less work repetition.

Proposals

  • Prioritise zero-tolerance to flaky tests from development side
  • If flaky tests couldn't be easily fixed, possibly move them away from PR checks to after-merge checks.
  • Create a single unified workflow, and research the performance impact of it.
  • Explore ways to use a sane scripting language for CI, not Shell. That's for a separate issue, maybe.
@0x009922 0x009922 added question Further information is requested iroha2-dev The re-implementation of a BFT hyperledger in RUST CI labels Apr 26, 2024
@DCNick3
Copy link
Contributor

DCNick3 commented Apr 26, 2024

And these are particular flaky tests:
integration::extra_functional::offline_peers::genesis_block_is_committed_with_some_offline_peers
integration::extra_functional::unstable_network::soft_fork

Agree, I often find those particular tests failing. Though, a lot of client-cli-tests are also very flaky =(. I noticed, in particular, test_burn_asset_for_account_in_same_domain and test_register_account_with_invalid_domain, but there are probably more

The nice things about the multiple workflows is that they are ran in parallel and fail independently (that, in particular, is probably why unstable workflow is a thing).

@AlexStroke
Copy link
Contributor

Prioritise zero-tolerance to flaky tests from development side
If flaky tests couldn't be easily fixed, possibly move them away from PR checks to after-merge checks.

We need to think and implement a definitive solution that ensures tests are not flaky by fundamentally resolving synchronization issues. For example, in the Python SDK, @SamHSmith and I are working on a 'submit and block' function in the client. This function will only return once a transaction has been fully processed and confirmed on all peers, regardless of the machine and resources the tests are running on. Maybe it should be some more centralized solution so that you don't have to implement it in every client and SDK.

@AlexStroke
Copy link
Contributor

Prioritise zero-tolerance to flaky tests from development side
If flaky tests couldn't be easily fixed, possibly move them away from PR checks to after-merge checks.

We need to think and implement a definitive solution that ensures tests are not flaky by fundamentally resolving synchronization issues. For example, in the Python SDK, @SamHSmith and I are working on a 'submit and block' function in the client. This function will only return once a transaction has been fully processed and confirmed on all peers, regardless of the machine and resources the tests are running on. Maybe it should be some more centralized solution so that you don't have to implement it in every client and SDK.

https://github.com/hyperledger/iroha-python/pull/196/files

@0x009922 0x009922 self-assigned this May 30, 2024
@0x009922
Copy link
Contributor Author

0x009922 commented May 31, 2024

@nxsaken do you think your work on #4500 will address flakyness issue?

@nxsaken
Copy link
Contributor

nxsaken commented May 31, 2024

@0x009922 I keep it in mind, but not sure yet because I haven't looked into why the tests are flaky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI iroha2-dev The re-implementation of a BFT hyperledger in RUST question Further information is requested
Projects
None yet
5 participants