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

bitswap: reenable unit tests #327

Closed
BigLep opened this issue May 31, 2023 · 4 comments · Fixed by #740
Closed

bitswap: reenable unit tests #327

BigLep opened this issue May 31, 2023 · 4 comments · Fixed by #740
Labels
good first issue Good issue for new contributors help wanted Seeking public contribution on this issue topic/bitswap

Comments

@BigLep
Copy link
Contributor

BigLep commented May 31, 2023

Done Criteria

Bitswap has tests reenabled such that we remove judgement from developers. CI must be green before we merge.

Why Important

In the absence of clear signal, developers have sometimes skipping CI checks, which in at least one instance caused a bug to go through.

Notes

This isn't intended to be a "fix the flaky tests" effort. For now we can just rerun the flaky tests X times. We need to get to a place though where all tests have to pass before we merge.

Internal conversation on this: https://filecoinproject.slack.com/archives/C04SWLJ2QQ4/p1684434613517719

@BigLep BigLep added need/triage Needs initial labeling and prioritization topic/bitswap and removed need/triage Needs initial labeling and prioritization labels May 31, 2023
@Jorropo Jorropo added good first issue Good issue for new contributors help wanted Seeking public contribution on this issue labels Jun 2, 2023
@Jorropo
Copy link
Contributor

Jorropo commented Jun 2, 2023

What we want to do is going through trying to remove the tests.Flaky call from the bitswap/... tests and see what tests reliably pass and which one do not.

Some flaky tests will be very hard to fix, it is fine to leave tests.Flaky for them in some individual PRs, even if the target is to remove all usage of tests.Flaky to fix this issue a PR that leave some but increase code coverage is still awesome.

We can view coverage progress here: https://app.codecov.io/gh/ipfs/boxo/tree/main/bitswap

@BigLep
Copy link
Contributor Author

BigLep commented Jun 30, 2023

Per sync with PL EngRes Bedrock on 2023-06-30, we'll update with where this sits in the queue (and expected completion date) by 2023-07-07.

@BigLep BigLep moved this to 🥞 Todo in IPFS Shipyard Team Jul 6, 2023
@BigLep
Copy link
Contributor Author

BigLep commented Jul 10, 2023

I signed off on 2023-07-07 without updating this - my bad.
We expect to complete this this week.
Before doing this work, @Jorropo is tackling the in-progress fixes for ipfs/kubo#9927 and #209 (comment)

@BigLep BigLep moved this from 🥞 Todo to 🏃‍♀️ In Progress in IPFS Shipyard Team Aug 3, 2023
@BigLep
Copy link
Contributor Author

BigLep commented Oct 5, 2023

2023-10-05 conversation:

  • @Jorropo to summarize in this issue what tests are disabled and explain why those tests are disabled. We believe (but need to confirm) that this is all for a particular bug we aren't going to fix currently.

@Jorropo Jorropo removed their assignment Mar 4, 2024
gammazero added a commit that referenced this issue Dec 4, 2024
Recent improvements in bitswap should stabalize these tests.

Closes #327
gammazero added a commit that referenced this issue Dec 4, 2024
Recent improvements in bitswap should stabalize these tests.

Closes #327
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good issue for new contributors help wanted Seeking public contribution on this issue topic/bitswap
Projects
No open projects
Status: 🏃‍♀️ In Progress
Development

Successfully merging a pull request may close this issue.

2 participants