Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(orm): gRPC codes for save errors #11386
feat(orm): gRPC codes for save errors #11386
Changes from 6 commits
47f077177d58b7a0a0ce295e184587ba6d7d8ef8b84ea80b3f3aa16005dddf0File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Please don't introduce new testing frameworks to the project.
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.
Why?
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.
There's no reason for it. Everything here is equally well expressed as conventional table-based Go unit tests.
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.
Each new dependency added to
go.modmust be default deny. The onus is on the author to justify its value.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.
The goal is readability. We've decided internally to try this type of testable documentation. If it doesn't provide value, we won't continue with it
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.
It might be useful to get an ADR on testing up and which way things are going. We all understand there is an issue with testing in the cosmos-sdk that leads to messed up ci pipelines and not having full coverage.
I thought there was agreement on this from the Regen side, but seems there are conflicting opinions.
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 have clearly misunderstood the purpose of PRs in this project. My apologies. I'll refrain from commenting or contributing any further.
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.
Folks on the regen ledger side have looked into this more and that is where our devs are more aligned around this approach @marbar3778. We can prepare something a bit more formal if that helps. Although the reality is we don't usually do too thorough of an audit of 3rd party test dependencies as is...
That's the point. There is no English language spec. That's precisely what this introduces
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.
@marbar3778 let's continue discussion in #11356. This has two approvals but I will hold off on merging until I get your approval @AmauryM. Will be in touch with both of you around how we want to move forward
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.
For the matter of the fact: most of the Cosmos SDK tests are not readable. Cucumber style tests are way more readable (few orders of magnitude +1).
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.