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: Revert "Improve BFT Tests Stability (#1385)" #1441

Merged
merged 1 commit into from
Dec 17, 2023

Conversation

gfanton
Copy link
Member

@gfanton gfanton commented Dec 15, 2023

This PR reverts #1385 following @jaekwon #1320 (comment)

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

This reverts commit 63ff15b.

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Dec 15, 2023
Copy link

codecov bot commented Dec 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eef0c98) 56.32% compared to head (8392243) 56.06%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1441      +/-   ##
==========================================
- Coverage   56.32%   56.06%   -0.26%     
==========================================
  Files         422      421       -1     
  Lines       65699    65457     -242     
==========================================
- Hits        37003    36700     -303     
- Misses      25815    25891      +76     
+ Partials     2881     2866      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gfanton gfanton changed the title Revert "fix: Improve BFT Tests Stability (#1385)" fix: Revert "Improve BFT Tests Stability (#1385)" Dec 15, 2023
Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

I am blocking the merge because it will reintroduce the annoying CI. We need time to make the final fix.

However, let's keep this PR open to make sure to address the underlying problem instead of ignoring it.

I'll mark this pull request as blocking the launch, as it was already the case for #1320.

@moul
Copy link
Member

moul commented Dec 17, 2023

After further discussion with Jae, it is evident that fixing this issue promptly is more important than delaying it.

The usual concern is that if we try to ignore such problem, it may be forgotten later. However, what makes this issue unique is that it used to occur less frequently in the past. Although I don't have precise numbers, the occurrence rate was likely around 1 in 10, but now it has increased to 5 in 10. Therefore, the situation has worsened over time. By hiding the issue, we not only postpone the fix, but also allow the problem to grow silently.

I will merge this pull request now, which will inevitably cause the CI to become unstable again. However, by doing so, we can ensure that it is prioritized and fixed. Additionally, we should consider checking the other unstable tests now rather than later, specifically #202.

@moul moul merged commit 6cabad4 into gnolang:master Dec 17, 2023
179 of 181 checks passed
@moul moul mentioned this pull request Dec 17, 2023
gfanton added a commit to moul/gno that referenced this pull request Jan 18, 2024
This PR reverts gnolang#1385 following @jaekwon
gnolang#1320 (comment)

<!-- please provide a detailed description of the changes made in this
pull request. -->

<details><summary>Contributors' checklist...</summary>

- [ ] Added new tests, or not needed, or not feasible
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [ ] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related
Projects
Status: 🚀 Needed for Launch
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants