-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
feat: Add legacy simapp testing via build tags #12121
Conversation
@aaronc @kocubinski @alexanderbez would like your input on this 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine for now, we can run these tests hourly or something just not in every pr to avoid clogging the system
Gotcha, ok, will work on this and then will ask for help to add it to the CI :) |
lmk I can knock out the ci things quickly |
…/simapp-legacy-test
…/simapp-legacy-test
This is ready for review. We have to decide when to run the legacy simapp tests. To do so we just add the build tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly would prefer to have this under app/legacy
if possible? Otherwise, build tags is OK
Wouldn't that mean that we need to re-write all tests? I'm open to other options but what I really like of this approach is that we would be running the exact same tests for both app wiring and legacy app.go. |
We should use the version of app.go before app wiring started. If it's under legacy and not a build tag tests can't actually be run. Eventually we probably don't want to maintain the legacy form but it's useful for maybe one release? |
I went back in history and picked up the last commit before your first PR got merged, that's what you mean? |
I mean keep the build tag but just move it under a legacy package. |
@marbar3778 should we merge and you do the CI in a separate PR or want to work on it on this same branch? |
It wouldn't be possible to run the same tests then afaik |
ill knock it out in this pr quickly |
name: Legacy App Testing | ||
on: | ||
schedule: | ||
- cron: "0 0,12 * * *" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set it to run twice a day, is that okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need some changes, as we have changed NewSimApp
arguments.
## Description This is a PoC just to know what the team thinks of it. Pros: - No need to re-write tests - We make sure that we are running all the same tests on both files Cons: - We need to run tests twice (although we would be running them twice if we copy&paste all of them) Closes: cosmos#12068 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)
Description
This is a PoC just to know what the team thinks of it.
Pros:
Cons:
Closes: #12068
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change