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: add support for dynamic user addresses in txtar tests #1581

Closed

Conversation

deelawn
Copy link
Contributor

@deelawn deelawn commented Jan 24, 2024

This feature makes it possible to reference addresses of users that were dynamically added using the adduser command. The modified adduser.txtar file contains an example of this and the modified doc file contains instructions how to use it.

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 gfanton as a code owner January 24, 2024 19:37
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Jan 24, 2024
@deelawn deelawn requested a review from a team January 24, 2024 19:42
Copy link

codecov bot commented Jan 24, 2024

Codecov Report

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

Comparison is base (d1ead00) 55.80% compared to head (1dd23d8) 55.80%.

Files Patch % Lines
gno.land/pkg/integration/testing_integration.go 88.46% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1581      +/-   ##
==========================================
- Coverage   55.80%   55.80%   -0.01%     
==========================================
  Files         435      435              
  Lines       66055    66073      +18     
==========================================
+ Hits        36862    36869       +7     
- Misses      26320    26328       +8     
- Partials     2873     2876       +3     
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.

@zivkovicmilos zivkovicmilos self-requested a review January 25, 2024 10:26
@thehowl
Copy link
Member

thehowl commented Jan 26, 2024

@gfanton I'll leave you to do the first review, ping me if you need a second one.

@gfanton
Copy link
Member

gfanton commented Jan 26, 2024

I'm fine with the code, but I don't really understand why we should use withuser instead of an environment variable, which seems more straightforward to me.

adduser test8
withuser test8 gnokey maketx call -pkgpath gno.land/r/bar -func Render -args 'USERADDRESS' 

vs

adduser test8
gnokey maketx call -pkgpath gno.land/r/bar -func Render -args $ADDRESS_test8

Additionally, withuser doesn't seem very composable and can disrupt copy/paste actions. I'm also concerned that if we start using this kind of command, we might drift away from the original gnokey command usage.

@deelawn
Copy link
Contributor Author

deelawn commented Jan 29, 2024

Thanks @gfanton . You're 100% correct. I forgot it exports this environment variable 🤦‍♂️. Closing this...

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
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants