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 r/sys/params + params genesis support #3003

Open
wants to merge 79 commits into
base: master
Choose a base branch
from

Conversation

moul
Copy link
Member

@moul moul commented Oct 22, 2024

Depends on #2920
Depends on #3003 (cherry-picked)
Blocking #2911

Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
@moul moul self-assigned this Oct 22, 2024
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Oct 22, 2024
Signed-off-by: moul <[email protected]>
@moul
Copy link
Member Author

moul commented Oct 26, 2024

I believe that Codecov has false negatives. It flags lines that are actually tested at least twice.

Signed-off-by: moul <[email protected]>
@moul moul marked this pull request as ready for review October 26, 2024 02:51
@moul moul requested review from jaekwon, zivkovicmilos and gfanton and removed request for jaekwon October 26, 2024 02:51
Signed-off-by: moul <[email protected]>
@@ -59,6 +59,7 @@ func TestingNodeConfig(t TestingTS, gnoroot string, additionalTxs ...std.Tx) (*g

creator := crypto.MustAddressFromString(DefaultAccount_Address) // test1

params := LoadDefaultGenesisParamFile(t, gnoroot)
Copy link
Member Author

Choose a reason for hiding this comment

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

@gfanton, I think there is a problem with Codecov. Can you check why this line isn't reported?

Copy link
Member

Choose a reason for hiding this comment

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

Because it's only used by txtar files in gno.land, unfortunately, the gno.land txtar doesn't set up coverage yet. I suggest you ignore this for now. Ideally, I would like to take some time centralize the logic of txtar from both gnovm and gno.land.

Copy link
Member

Choose a reason for hiding this comment

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

I see you want to avoid losing too much coverage by taking advantage of txtar. Let me see if I can find a shortcut.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm just looking for my PR to be green, because I wrote tests.

Copy link
Member

Choose a reason for hiding this comment

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

@moul

So it looks like your issue has nothing to do with txtar.

By default, Go's test coverage tool only considers the packages that are directly imported by the package containing the tests. This means that if you have a package dependency chain like:

  • Package A (contains tests) imports Package B
  • Package B imports Package C

Coverage will be calculated for Package A and Package B, but Package C will not be included in the coverage report unless explicitly specified. This is because Go's default behavior is to only instrument the packages directly involved in the test run.

In your case:

  • Package A is the package that holds the test file in gno.land/cmd/gnoland
  • Package B is gno.land/pkg/integration, and it's the package that will actually run the txtar
  • Package C is the package that contains LoadDefaultGenesisParamFile and will not be covered by your test

There are 2 possible fixes for this:

  • The first one, more global, is to tell Go to test and cover all packages under gno.land using -coverpkg=github.com/gnolang/gno/gno.land/.... This way, all packages and sub-package imports will be taken into consideration; it can be a good solution but at the expense of slower tests.
  • The second solution, more specific to your PR, is to reduce the chain dependency of your test. That means simply moving your params.txtar from gno.land/cmd/gnoland/testadata to gno.land/pkg/integration/testdata.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm in favor of option 1, unless "slower" means "way too much slower."

Copy link
Member Author

Choose a reason for hiding this comment

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

Something like this: #3088?

Copy link
Member

Choose a reason for hiding this comment

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

Something like this: #3088?

see my comment #3088 (comment)

@moul
Copy link
Member Author

moul commented Oct 30, 2024

Ready for review. The CI is red, but the new code is tested.
The conflict is easy to fix, I'll fix it soon.
Blocking #2911.

thehowl pushed a commit that referenced this pull request Oct 31, 2024
Closer to the current `// Realm:` output, and more git-friendly.

Bigger example in #3003.

Signed-off-by: moul <[email protected]>
@Kouteki Kouteki added the in focus Core team is prioritizing this work label Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in focus Core team is prioritizing this work 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: No status
Status: 📥 Inbox
Status: In Review
Development

Successfully merging this pull request may close these issues.

3 participants