-
Notifications
You must be signed in to change notification settings - Fork 364
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: gno test
support /...
pattern
#1078
Conversation
gno test
argsgno test
support /...
pattern
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, but take comments into accounts, thank you
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.
Can you please add some unit tests for this functionality you've introduced?
It seems like there are a lot of edge-cases, and I'm not sure how you've verified the functionality, apart from manually running it over a data set or comparing outputs for a few cases.
Not to mention that the PR decreases the overall coverage we have:
https://app.codecov.io/gh/gnolang/gno/pull/1078
@moul I propose we start adding a CI rule that stops a PR from being merged if it does not increase or maintain coverage at a certain margin
What if we configure codecov to comment on PRs instead of auto-blocking them? This way, we can monitor coverage without hindering the PR process. |
@moul I also believe that we should have the responsibility to not merge uncovered code upstream, because it opens the door to a bad practice of "fixing it later" (or much more likely, never). It's part of a bigger issue we have in the repo, which is a non-standardized testing suite. I am not a huge fan of testing functionality by a command output as we're doing in this PR, and in other PRs (that have since been merged). Overall, I want to push for greater accountability and responsibility of upstream changes, not just because we are approaching a launch (and many launches after that), but also because I want us to maintain a codebase that is maintainable by us and the community, and a codebase that won't come back at us like a boomerang down the road I would love to hear other opinions as well on this issue, @gfanton @ajnavarro @thehowl |
For me, the code itself is the least important part of a PR. Code is a mere tool to accomplish a better state of the program. To be able to verify that the state of the program/application will be better with the PR merged, we need ways to check it, and we need to make that change discoverable for the users. To check it, we need reproducible tests. Depending on the case: golden tests/regression tests/integration tests/unit tests... To discover the change: we need an entry on our changelog and/or go doc and user docs. Those are the minimum parts IMHO of a PR in priority order:
If we want to verify further, we have to add benchmarks checking our new code performance too, but maybe we are too far away from that yet. |
Alright, let's experiment with enabling codecov by default for a while. I'm wary it might lead to valuable work being delayed. While I'm a proponent of atomic PRs with integrated tests, I also value dedicated sessions for refining tests (frameworks and units alike). So, while I'm open to temporarily blocking PRs based on codecov, I suspect we might eventually revert to just enabling comments. (easy to change). Tests, as you said and in my opinion, are possibly even more critical than the code they assess. Regarding documentation: Let's also raise the bar, but I believe in focusing on inline code documentation or making the code self-explanatory. Let's prioritize embedded documentation over separate standalone documentation within the context of PRs, keeping the latter as an independent area of improvement. An ideal PR, for me, is one that impacts only a specific folder without modifying root files. By the way, check out #1059. On the changelog topic: I concur with its significance. However, ensuring clear and comprehensive PR titles and descriptions should suffice. While maintaining side changelog files might have value, I'd lean toward not imposing too many requirements on individual PRs. It could make sense to make releases parties where we update changelog files. Ultimately, my advocacy lies in:
I see code generation as a significant tool to aid in these goals, allowing our primary focus on the aforementioned aspects. Edit: #1120 |
To me, one of the most crucial aspects is enforcing rules within the CI. Code coverage, if not mandated, is often overlooked due to complacency. While exceptions exist, they should be made explicit, similar to using a Regarding non-standard testing suites, specifically golang's txtar tests, I'm relatively unfamiliar with this approach. However, I believe they can complement standard tests. We should always prioritize standard tests, but from what I understand, txtar tests could be useful for specific scenarios, such as testing edge cases or situations involving reading from a directory tree or multiple files. This can make tests more readable and maintainable. Nonetheless, standard tests should never be relegated to a secondary option. As example, for this specific PR, using txtar doesn't seem appropriate since pattern matching can be tested with standard tests, as shown in the original test at https://tip.golang.org/src/cmd/internal/pkgpattern/pat_test.go. |
Txtar aims to create integration tests that closely mirror real-world usage, employing the exact tools that end-users will utilize. Consider a scenario where a company boasts 100% code coverage. Everything appears perfect, but the sign-up flow - registering and then logging in - was overlooked. Developers were already logged in, so while CI tests passed, no new user could register. The primary objectives of txtar are:
Currently, txtar leverages the compiled binary, which enriches the mock-free testing experience. I propose we rename |
See #1078 (review) Frankly, I'm always cautious when working with configuration files that can only be fully validated after merging and opening a new PR. I've consulted the documentation and also referenced popular codecov files from other repositories for guidance. Reference: https://docs.codecov.com/docs/codecovyml-reference
791c1ca
to
cef6b80
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1078 +/- ##
==========================================
+ Coverage 47.79% 47.86% +0.06%
==========================================
Files 369 369
Lines 62710 62764 +54
==========================================
+ Hits 29975 30040 +65
+ Misses 30312 30299 -13
- Partials 2423 2425 +2
☔ View full report in Codecov by Sentry. |
Code introduced in this PR has still not been properly covered, please see the CodeCov report associated with this PR. Please ping me when the issue has been resolved 🙏 |
Related with #1215. |
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.
Thank you for the fixes and changes 🙏
Clear to go 🚀
BREAKING CHANGE: Altered behavior of the `gno test` command Adds support for `/...` pattern in `gno test` command. Now args can have `/...` pattern in the directory path. Using `gno test ./path/to/pkg` would trigger the execution of test files solely within the specified package directory, excluding any subdirectories like `./path/to/pkg/subpkg`. To execute test files within subdirectories as well, use `gno test ./path/to/pkg/...` It supports all variations of `/...` such as `./path/.../pkg`, `./.../pkg`, ,`./.../path/...` and more <details><summary>Contributors' checklist...</summary> - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [x] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details>
BREAKING CHANGE: Altered behavior of the `gno test` command Adds support for `/...` pattern in `gno test` command. Now args can have `/...` pattern in the directory path. Using `gno test ./path/to/pkg` would trigger the execution of test files solely within the specified package directory, excluding any subdirectories like `./path/to/pkg/subpkg`. To execute test files within subdirectories as well, use `gno test ./path/to/pkg/...` It supports all variations of `/...` such as `./path/.../pkg`, `./.../pkg`, ,`./.../path/...` and more <details><summary>Contributors' checklist...</summary> - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [x] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details>
BREAKING CHANGE: Altered behavior of the `gno test` command Adds support for `/...` pattern in `gno test` command. Now args can have `/...` pattern in the directory path. Using `gno test ./path/to/pkg` would trigger the execution of test files solely within the specified package directory, excluding any subdirectories like `./path/to/pkg/subpkg`. To execute test files within subdirectories as well, use `gno test ./path/to/pkg/...` It supports all variations of `/...` such as `./path/.../pkg`, `./.../pkg`, ,`./.../path/...` and more <details><summary>Contributors' checklist...</summary> - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [x] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details>
BREAKING CHANGE: Altered behavior of the
gno test
commandAdds support for
/...
pattern ingno test
command. Now args can have/...
pattern in the directory path.Using
gno test ./path/to/pkg
would trigger the execution of test files solely within the specified package directory, excluding any subdirectories like./path/to/pkg/subpkg
.To execute test files within subdirectories as well, use
gno test ./path/to/pkg/...
It supports all variations of
/...
such as./path/.../pkg
,./.../pkg
, ,./.../path/...
and moreContributors' checklist...
BREAKING CHANGE: xxx
message was included in the description