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.

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: new txtar command adduser #1471

Merged
merged 14 commits into from
Jan 10, 2024

Conversation

deelawn
Copy link
Contributor

@deelawn deelawn commented Dec 20, 2023

As I've started to think about creating txtar tests for contracts with logic depending on contract calls from multiple users and durations since previous contract calls, the addition of the adduser command seems to be necessary.

Contributors' checklist...
  • 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, if any. More info here.

@deelawn deelawn requested a review from a team as a code owner December 20, 2023 00:35
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Dec 20, 2023
@deelawn deelawn requested a review from gfanton December 20, 2023 00:36
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (968f89e) 55.87% compared to head (211c82c) 55.65%.
Report is 23 commits behind head on master.

Files Patch % Lines
gno.land/pkg/integration/testing_integration.go 69.56% 14 Missing and 7 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1471      +/-   ##
==========================================
- Coverage   55.87%   55.65%   -0.23%     
==========================================
  Files         430      431       +1     
  Lines       65618    67201    +1583     
==========================================
+ Hits        36667    37402     +735     
- Misses      26083    26866     +783     
- Partials     2868     2933      +65     
Flag Coverage Δ
go-1.21.x ∅ <ø> (∅)
misc ∅ <ø> (∅)
misc-_test.genstd ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@moul
Copy link
Member

moul commented Dec 20, 2023

Related with #1269

Copy link
Member

@gfanton gfanton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide more details or/and example regarding the use of the sleep command? As mentioned in #1471 (comment) , I'm a bit apprehensive about introducing this command.

@deelawn deelawn changed the title feat: new txtar commands adduser and sleep feat: new txtar command adduser Dec 21, 2023
@zivkovicmilos zivkovicmilos self-requested a review December 21, 2023 08:37
Copy link
Member

@gfanton gfanton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

maybe you can move txtar tests inside gno.land/pkg/integration/testdata as they are only here to test the adduser feature.

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really cool feature 💯

I've left some comments that should be addressed before we proceed, namely the initial accounts that are available to node starts with txtar

gno.land/pkg/integration/testing_integration.go Outdated Show resolved Hide resolved
gno.land/pkg/integration/testing_integration.go Outdated Show resolved Hide resolved
gno.land/pkg/integration/doc.go Outdated Show resolved Hide resolved
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking a look at the comments 🙏

I've opened up an issue to discuss how we orchestrate txtar nodes, to avoid further blocking this PR:
#1484 (comment)

@deelawn deelawn merged commit 2acf839 into gnolang:master Jan 10, 2024
180 of 181 checks passed
@deelawn deelawn deleted the feat/txtar-sleep-adduser branch January 10, 2024 21:12
gfanton pushed a commit to moul/gno that referenced this pull request Jan 18, 2024
<!-- please provide a detailed description of the changes made in this
pull request. -->
As I've started to think about creating txtar tests for contracts with
logic depending on contract calls from multiple users and durations
since previous contract calls, the addition of the `adduser` command
seems to be necessary.
<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
- [ ] 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>

---------

Co-authored-by: Hariom Verma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants