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.

Sign up for GitHub

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

Jump to bottom

Excercise panic cases in tests for UpgradeOpen/UpgradeRestore #5471

Closed
2 of 3 tasks
DimitrisJim opened this issue Dec 20, 2023 · 2 comments · Fixed by #5550
Closed
2 of 3 tasks

Excercise panic cases in tests for UpgradeOpen/UpgradeRestore #5471

DimitrisJim opened this issue Dec 20, 2023 · 2 comments · Fixed by #5550
Assignees
@neitdung
Labels
27-interchain-accounts testing Testing package and unit/integration tests type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.

Comments

@DimitrisJim
Copy link
Contributor

DimitrisJim commented Dec 20, 2023 •
edited
Loading

Ref review comment:

Suppose there will never an expError because the func can only panic in one case.

It should be possible to set some mock app or middleware as the app which doesn't implement the interface. Maybe could reuse something from #5406 with expPanics, where instead of using nil we use a mock that doesn't implement the interface:

if expPanics {
    cbs = controller.NewIBCMiddleware(ibcmock.NewBlockUpgradeMiddleware(), suite.chainA.GetSimApp().ICAControllerKeeper)
}

Happy to do it in a follow up if we go with the mock upgrade middleware

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@DimitrisJim DimitrisJim added testing Testing package and unit/integration tests 27-interchain-accounts type: refactor Architecture, code or CI improvements that may or may not tackle technical debt. labels Dec 20, 2023
@DimitrisJim DimitrisJim mentioned this issue Dec 20, 2023
Merged
9 tasks
@neitdung
Copy link
Contributor

Hi sir @DimitrisJim, can I pick up this issue?

@DimitrisJim
Copy link
Contributor Author

can do @neitdung! I'll assign ya now.

@neitdung neitdung mentioned this issue Jan 8, 2024
Merged
9 tasks
@mergify mergify bot closed this as completed in #5550 Apr 19, 2024
mergify bot pushed a commit that referenced this issue Apr 19, 2024
@neitdung
test: add panic cases in tests for UpgradeOpen (#5550)
247068b 
## Description


- Add panic test cases for `OnChanUpgradeOpen` and `OnChanUpgradeRestore`
- Reuse mock middleware as the app which doesn't implement the `UpgradeableModule` interface from [#5406](#5406)

closes: #5471 


### Commit Message / Changelog Entry

```text
test: add panic cases in tests when channel upgrade open or restore 
```

see the [guidelines](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) for commit messages. (view raw markdown for examples)




---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [x] Targeted PR against the correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#pull-request-targeting)).
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/main/docs/build/building-modules/11-structure.md) and [Go style guide](../docs/dev/go-style-guide.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/main/testing/README.md#ibc-testing-package).
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`).
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [x] Provide a [commit message](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) to be used for the changelog entry in the PR description for review.
- [ ] Re-reviewed `Files changed` in the Github PR explorer.
- [ ] Review `Codecov Report` in the comment section below once CI passes.



## Summary by CodeRabbit


- **Tests**
    - Enhanced testing for channel upgrades in interchain accounts, including new scenarios for error handling.
    - Added import for `cosmossdk.io/errors`.
    - Added `expPanic` field to test cases in `TestOnChanUpgradeOpen` function.
    - Updated the logic in `TestOnChanUpgradeOpen` to handle expected errors and panics based on test cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@neitdung neitdung

Labels
27-interchain-accounts testing Testing package and unit/integration tests type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: add panic cases in tests for UpgradeOpen decentrio/ibc-go
2 participants
@DimitrisJim @neitdung