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(txtar): add loadpkg command #1598

Merged
merged 26 commits into from
Feb 23, 2024
Merged

Conversation

gfanton
Copy link
Member

@gfanton gfanton commented Jan 29, 2024

address #1484

This PR introduces the loadpkg command to gnoland integration using txtar:

- Must be run before `gnoland start`.
- Loads a specific package from the example folder or from the working ($WORK) directory.
- Can be used to load a single package or all packages within a directory.
- If the target package has a `gno.mod`, all its dependencies (and their respective
  dependencies) will also be loaded.
- The command takes either one or two arguments. The first argument is the name of the package(s),
  and the second (optional) argument is the path to the package(s).
  Examples:
  -- # Load a package from the example packages directory:
  -- loadpkg gno.land/p/demo/ufmt
  -- # Load a package `./bar` from the current testscript's working directory with the name `gno.land/r/foobar/bar`:
  -- loadpkg gno.land/r/foobar/bar $WORK/bar
- If the path is not prefixed with the working directory, it is assumed to be relative to the
  examples directory.
- It's important to note that the load order is significant when using multiple `loadpkg`
  command; packages should be loaded in the order they are dependent upon.

Additionally, this PR disables the default automatic loading of the example folder to keep test requirements minimal.

Moreover, it remove other testX users as specified in #1484 and retaining only the test1 user. Additional users can be added as needed using the adduser command.

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.

@gfanton gfanton self-assigned this Jan 29, 2024
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Jan 29, 2024
Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Attention: Patch coverage is 54.47154% with 56 lines in your changes are missing coverage. Please review.

Project coverage is 44.56%. Comparing base (16c7c2e) to head (bca5a19).

Files Patch % Lines
gno.land/pkg/integration/testing_integration.go 63.80% 27 Missing and 11 partials ⚠️
gno.land/pkg/gnoland/genesis.go 0.00% 18 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1598      +/-   ##
==========================================
- Coverage   47.41%   44.56%   -2.85%     
==========================================
  Files         385      460      +75     
  Lines       61319    67903    +6584     
==========================================
+ Hits        29074    30264    +1190     
- Misses      29826    35102    +5276     
- Partials     2419     2537     +118     

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

Signed-off-by: gfanton <[email protected]>
Copy link
Contributor

@deelawn deelawn left a comment

Choose a reason for hiding this comment

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

Looks good to me other than the few minor comments.

@deelawn
Copy link
Contributor

deelawn commented Jan 30, 2024

One more thing -- I noticed in the issue referenced here that it also says the premined addresses and balances should be removed, maybe with the exception of test1. Is that a change that should be included in this PR as well?

@gfanton
Copy link
Member Author

gfanton commented Jan 30, 2024

One more thing -- I noticed in the issue referenced here that it also says the premined addresses and balances should be removed, maybe with the exception of test1. Is that a change that should be included in this PR as well?

oh, yes you right! I will remove this in this PR, as it's not needed anymore with the adduser command !

@gfanton gfanton marked this pull request as ready for review January 30, 2024 15:25
@gfanton gfanton requested a review from moul as a code owner January 30, 2024 15:25
Signed-off-by: gfanton <[email protected]>
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.

Amazing 👏

No specific comments, just a typo and an observation. Looks great 💯

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/testing_integration.go Outdated Show resolved Hide resolved
gno.land/pkg/integration/testing_integration.go Outdated Show resolved Hide resolved
Copy link
Contributor

@harry-hov harry-hov left a comment

Choose a reason for hiding this comment

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

Just left a few suggestions that might be worth taking a look. Rest LGTM 💯

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/testing_integration.go Outdated Show resolved Hide resolved
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/testdata/use_work.txtar Outdated Show resolved Hide resolved
gno.land/pkg/gnoland/genesis.go Outdated Show resolved Hide resolved
@tbruyelle
Copy link
Contributor

Great addition, but why calling it use ? addpkg sounds more appropriate to me.

@moul
Copy link
Member

moul commented Feb 8, 2024

Do we keep at least one oldskool gnokey maketx addpkg -pkgdir $WORK/bar -pkgpath gno.land/r/foobar/bar -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1?

"Use" looks better, but since it's an integration test system, we need to explicitly keep at least one "using gnokey".

@thehowl
Copy link
Member

thehowl commented Feb 8, 2024

So, @gfanton, from the review meeting:

  • Let's consider whether it makes sense to change the name from use to something more descriptive. addpkg is an option, but since this is not an alias but rather adding the packages at genesis, maybe copy? (so like dockerfiles)
    • the comment about documenting usage before gnoland start

thanks!

Signed-off-by: gfanton <[email protected]>
@gfanton
Copy link
Member Author

gfanton commented Feb 12, 2024

@tbruyelle @moul @thehowl I think copy would have been a good choice if packages were loaded individually. However, for practical reasons, it also loads package dependencies recursively based on the gno.mod file of the package (and sub-packages). (i've updated doc.go to mention this)
Even though it can be a bit confusing, I would say it's more accurate to use addpkg or loadpkg. Alternatively, we could simply use import, but it could be ambiguous compared to gno/go import.
wdyt ?

@thehowl
Copy link
Member

thehowl commented Feb 22, 2024

From the review meeting: agreed on load

@gfanton gfanton changed the title feat(txtar): add use command feat(txtar): add loadpkg command Feb 23, 2024
@gfanton gfanton merged commit 6ff519f into gnolang:master Feb 23, 2024
185 of 186 checks passed
@gfanton gfanton deleted the feat/txtar-use branch February 23, 2024 08:32
leohhhn pushed a commit to leohhhn/gno that referenced this pull request Feb 29, 2024
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.

7 participants