-
Notifications
You must be signed in to change notification settings - Fork 375
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 txtar driver for gnoland integration test #1117
Conversation
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 overall looks good, ping me again when you'll need another review.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1117 +/- ##
==========================================
+ Coverage 47.31% 47.59% +0.27%
==========================================
Files 367 369 +2
Lines 62118 62549 +431
==========================================
+ Hits 29394 29770 +376
- Misses 30325 30370 +45
- Partials 2399 2409 +10
☔ View full report in Codecov by Sentry. |
fe2f1d5
to
2b7a2fa
Compare
** as requested, this is a squash of the following commits: - feat: use commands.IO - chore: cleanup gnoland output using context logger - chore: cleanup integration test - chore: cleanup - feat: add test data example - chore: cleanup - fix: concurent run of gnoland - chore: cleanup - fix: cleanup - fix: docker integration test - chore: cleanup root dir and validate config - fix: correctly update command io argument - chore: cleanup Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
d47e2f4
to
ad175a8
Compare
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
@thehowl I agree that deeper documentation is needed, especially with an entrypoint from |
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
147aaea
to
b8bd6b5
Compare
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Ok, you got me, I made the |
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.
Approving to unblock. Mostly minor things :)
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
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 code looks great, but I have hard feelings about this kind of non-standard tests.
They tend to be the only way the codebase is tested, not using unit tests, so eventually, devs are not forced to make their codebase more testable (code easy to test is also better code and easy to understand and refactor), they just have to add some of these integration tests.
An example of what I'm talking about:
https://github.com/ipfs/kubo/tree/master/test/sharness
IPFS/Kubo had thousands of this kind of integration tests, and the core team spent thousands of hours fixing them when they broke. It is not an easy task due to strange errors (from the code and from the custom framework) that require hours to be able to know what is happening. The core team is getting rid of them bit by bit now.
Non-standard ways of testing (or doing anything, really) make the process more time-consuming for both parties, the one creating the tests, and the one fixing it when it breaks.
Summoning @thehowl and @moul for feedback about this concern.
@ajnavarro I was re-thinking about this and when compared to bash script-based 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.
To say this is a step in the right direction is an understatement 🙂
It's more like a leap in how we verify functionality.
Having said this, I've noted elsewhere I'm not a huge fan of this type of testing, but I am also fully aware we need to test e2e flows (especially with commands). The node orchestration will come in handy for lots of other efforts.
Thank you for the detailed documentation and explanations 🙏
|
||
func (c *AppOptions) validate() error { | ||
if c.Logger == nil { | ||
return fmt.Errorf("no logger provided") |
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.
Nitpick: this can be a regular errors.New
, since nothing is being formatted
Same nit for the other check in validate
@ajnavarro I tend to agree that in general we need to have a larger testing efforts such that when we add tests, we need to add them first and foremost at the unit level, then maybe at the integration level. That said, since we're coming from a codebase we've "inherited", unless somebody takes up the task of precisely testing and refactoring many of the components that currently don't have tests -- with experience with things we've encountered with GnoChess, I'd say one of the many that is in deep need of unit tests is GnoVM reference counting and realm persistance storage -- I don't think we can achieve effective good unit testing in the short term. txtar will be useful and in fact has already been useful in testing complex behaviour which for our current understanding cannot be easily replicated merely with unit tests. Taking into account your concerns, I propose that we don't consider txtar tests in our coverage reports (except maybe for gnovm/pkg/integration). This is already the case, but #1122 was due to change that. So I propose we change what we were doing in #1122 to only make the This way when a new change is added, containing txtar tests but not unit tests, then codecov will still complain that the coverage is not good enough -- and we agree (socially) that we normally don't merge PRs if the added changes are not covered. WDYT? |
Why not establish rules for the use of testscript ? For instance:
If none of these rules are true, then you shouldn't use testscript. |
Related with #1215. |
maybe make a PR on https://github.com/gnolang/gno/blob/master/docs/testing_guide.md to discuss? |
Resolves gnolang#1101. This PR establishes a base for creating `txtar` tests using a partial in-memory `gnoland` node. It supports: - `gnoland [start|stop]`: The node doesn't start automatically, which enables users to perform pre-configuration or pass custom arguments to the start command. - `gnokey`: Most of the commands should work. The `--remote`, `--insecure-password-stdin`, and `--home` flags are automatically set with the appropriate values to communicate with the node. - `sleep`: A simple helper to introduce a delay between actions, ensuring specific tasks complete before proceeding. Currently, only the "test1" user is automatically created in the default keybase directory (without a password). Default `gnoland` genesis balance and genesis transaction files are also load on start. You can find some examples of `txtar` in this directory: https://github.com/gfanton/gno/tree/feat/gnoland-txtar-driver/gno.land/cmd/gnoland/testdata `gnoland` logs aren't forwarded to stdout to avoid overwhelming informations in the tests. Instead, you can specify a log directory using the `LOG_DIR` environment variable or set `TESTWORK=true` to enable persistence in the `txtar` working directory. In either case, the path to the log file will be printed at the start if one of these environment variables is set. This also enables storing logs as artifacts on the CI for later examination. There is still a lot to do, but I believe this provides a good base for future iterations. <!-- please provide a detailed description of the changes made in this pull request. --> <details><summary>Contributors' checklist...</summary> - [ ] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [ ] Updated the official documentation or not needed - [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [ ] Added references to related issues and PRs - [ ] 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> --------- Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com> Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com> Co-authored-by: Morgan <git@howl.moe>
Resolves #1101.
This PR establishes a base for creating
txtar
tests using a partial in-memorygnoland
node. It supports:gnoland [start|stop]
: The node doesn't start automatically, which enables users to perform pre-configuration or pass custom arguments to the start command.gnokey
: Most of the commands should work. The--remote
,--insecure-password-stdin
, and--home
flags are automatically set with the appropriate values to communicate with the node.sleep
: A simple helper to introduce a delay between actions, ensuring specific tasks complete before proceeding.Currently, only the "test1" user is automatically created in the default keybase directory (without a password). Default
gnoland
genesis balance and genesis transaction files are also load on start.You can find some examples of
txtar
in this directory:https://github.com/gfanton/gno/tree/feat/gnoland-txtar-driver/gno.land/cmd/gnoland/testdata
gnoland
logs aren't forwarded to stdout to avoid overwhelming informations in the tests. Instead, you can specify a log directory using theLOG_DIR
environment variable or setTESTWORK=true
to enable persistence in thetxtar
working directory. In either case, the path to the log file will be printed at the start if one of these environment variables is set.This also enables storing logs as artifacts on the CI for later examination.
There is still a lot to do, but I believe this provides a good base for future iterations.
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description