Skip to content

Conversation

KolbyML
Copy link
Member

@KolbyML KolbyML commented Oct 15, 2025

Fixes NIT-4014

Problem

We fixed NIT-3997 "Regression in v3.8.x PopulateFeedBacklog depends on un-started InboxReader", but want to add a regression test that the fix did work, and a similar bug doesn't happen again

The fix was done in this PR

Solution

I added a regression test it passes on master (i.e. this branch), but fails before the original fix was merged.

To replicate this test failing more the fix was in place you can checkout this branch https://github.com/KolbyML/nitro/tree/old-add-regresion-test

Then run make go-test, cancel it after the test setup completes aka tests start running, then run go test ./system_tests -run TestRegressionInPopulateFeedBacklog -v -count=1 -timeout=10m and it will fail with error message

    common_test.go:687:  [] error populating feed backlog on startup: error getting message 1: failed to fetch batch mentioned by batch posting report: not started 

@KolbyML KolbyML force-pushed the new-add-regression-test branch from 968ebdc to 4f688d7 Compare October 15, 2025 17:33
builder.L2Info.GenerateAccount(userAccount)

// Guarantees that nodes will rely only on the feed to receive messages
builder.nodeConfig.BatchPoster.Enable = false
Copy link
Member

Choose a reason for hiding this comment

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

as we recently discovered, this will turn batch poster everywhere and should be adapted to #3829; bringing @ganeshvanahalli attention in case this PR will get merged first

@pmikolajczyk41
Copy link
Member

@KolbyML I'm afraid that the Aggregate test results fails because your fork doesn't have the codecov token secret. Probably you just have to use the original repo for PRs (though it's not a required check)

@KolbyML
Copy link
Member Author

KolbyML commented Oct 15, 2025

@pmikolajczyk41 ready for another look 🫡

pmikolajczyk41
pmikolajczyk41 previously approved these changes Oct 15, 2025
Copy link
Member

@pmikolajczyk41 pmikolajczyk41 left a comment

Choose a reason for hiding this comment

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

it's really great to have steps for reproduction, but (apologies for nitpicking), it'd be more straightforward to make old-add-regression-test branch differ from the actual branch by the fix we are testing (i.e., #3812); currently these branches have much more differences (KolbyML/nitro@new-add-regression-test...old-add-regresion-test and KolbyML/nitro@old-add-regresion-test...new-add-regression-test)

I run the test with and without fix and I confirm that the test does its job :D

@KolbyML
Copy link
Member Author

KolbyML commented Oct 15, 2025

it's really great to have steps for reproduction, but (apologies for nitpicking), it'd be more straightforward to make old-add-regression-test branch differ from the actual branch by the fix we are testing (i.e., #3812); currently these branches have much more differences (KolbyML/[email protected] and KolbyML/[email protected])

good to know I will keep that in mind for future PR's 🫡

I run the test with and without fix and I confirm that the test does its job :D

🥳

@eljobe eljobe enabled auto-merge October 16, 2025 14:25
@eljobe eljobe added this pull request to the merge queue Oct 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 16, 2025
@pmikolajczyk41 pmikolajczyk41 added this pull request to the merge queue Oct 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 16, 2025
@KolbyML KolbyML added this pull request to the merge queue Oct 16, 2025
Merged via the queue into OffchainLabs:master with commit 1026501 Oct 16, 2025
25 checks passed
@KolbyML KolbyML deleted the new-add-regression-test branch October 16, 2025 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants