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

Add a test to restart the app #3337

Closed
1 task
evan-forbes opened this issue Apr 17, 2024 · 3 comments · Fixed by #3556
Closed
1 task

Add a test to restart the app #3337

evan-forbes opened this issue Apr 17, 2024 · 3 comments · Fixed by #3556
Assignees
Labels
priority:high optional label to track the relative priority of planned items testing items that are strictly related to adding or extending test coverage WS: Maintenance 🔧 includes bugs, refactors, flakes, and tech debt etc WS: V2 ✌️ lemongrass hardfork related

Comments

@evan-forbes
Copy link
Member

evan-forbes commented Apr 17, 2024

Description

In the past, we've ran into at least two bugs that resulted in a panic after restarting (one being #3336).

Acceptance criteria

  • Unit test that starts stops and restarts a node. If a unit test format is not possible e2e test will also do
@evan-forbes evan-forbes added testing items that are strictly related to adding or extending test coverage ice-box this label is automatically applied to all issues. it is removed after starting work needs:triage needs:discussion item needs to be discussed as a group in the next sync. if marking an item, pls be prepped to talk labels Apr 17, 2024
@cmwaters
Copy link
Contributor

Good shout. it's hard to do a full reboot as a unit test. I think this would be better suited as a knuu test.

In cometbft's test they include perturbations which involve killing and restarting the docker instance

@evan-forbes
Copy link
Member Author

we might be able to with testnode if we manually call these and shutdown, then restart

cctx, stopNode, err := StartNode(tmNode, cctx)
require.NoError(t, err)
cctx, cleanupGRPC, err := StartGRPCServer(app, cfg.AppConfig, cctx)
require.NoError(t, err)
apiServer, err := StartAPIServer(app, *cfg.AppConfig, cctx)
require.NoError(t, err)

that might not work tho, in which case I agree, knuu is the way to go

@cmwaters
Copy link
Contributor

FYI, I think the upgrade tests already kill the node and restart it

@evan-forbes evan-forbes added priority:high optional label to track the relative priority of planned items WS: Maintenance 🔧 includes bugs, refactors, flakes, and tech debt etc and removed needs:priority needs:discussion item needs to be discussed as a group in the next sync. if marking an item, pls be prepped to talk ice-box this label is automatically applied to all issues. it is removed after starting work labels May 6, 2024
@cmwaters cmwaters self-assigned this Jun 12, 2024
@rootulp rootulp added the WS: V2 ✌️ lemongrass hardfork related label Jun 17, 2024
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:high optional label to track the relative priority of planned items testing items that are strictly related to adding or extending test coverage WS: Maintenance 🔧 includes bugs, refactors, flakes, and tech debt etc WS: V2 ✌️ lemongrass hardfork related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants