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

fix(cmd/gno): pass an ExecContext to MachineOptions in gno run #2856

Merged
merged 7 commits into from
Sep 27, 2024

Conversation

wyhaines
Copy link
Contributor

@wyhaines wyhaines commented Sep 26, 2024

This fixes: #2834

It tries to hew very closely to how the context is created under gno test so that there is consistency of results between code that is executed in the test and run contexts. Please let me know if there are any conventions for the codebase that I should have followed that I did not.

I hooked into the portion of the ExecRun() that was calling NewMachineWithOptions(), and instead followed a similar pattern to what is defined in test.go to configure the machine with a context that has reasonable defaults. These, except for the Chain ID (which is dev when running under gno test), are identical to the test settings.

This allows packages like gno.land/p/demo/entropy to work when code is executed with gno run, as well as any others which might try to access information only available from the context.

A simple piece of code to demonstrate the issue is below. This will fail without this change.

package foo

import (
  "fmt"
  "std"
)

func main() {
  fmt.Printf("GetHeight(): %d\n", std.GetHeight())
}

A test has been added to the tests for the run command, which tries to run code similar to the code above within the test.

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

This addresses this issue: gnolang#2834
It tries to hew very closely to how the context is created under `gno test` so that there is consistency of results between code that is executed in the test and run contexts.
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Sep 26, 2024
Copy link

codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.95%. Comparing base (577c462) to head (54d976c).
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2856      +/-   ##
==========================================
+ Coverage   60.92%   60.95%   +0.02%     
==========================================
  Files         564      564              
  Lines       75267    75380     +113     
==========================================
+ Hits        45854    45945      +91     
- Misses      26041    26049       +8     
- Partials     3372     3386      +14     
Flag Coverage Δ
contribs/gnodev 60.65% <ø> (-0.82%) ⬇️
contribs/gnofaucet 14.46% <ø> (ø)
gno.land 67.17% <ø> (ø)
gnovm 65.77% <100.00%> (+<0.01%) ⬆️
misc/genstd 80.54% <ø> (ø)
misc/logos 19.88% <ø> (-0.36%) ⬇️
tm2 62.10% <ø> (+<0.01%) ⬆️

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.

@wyhaines wyhaines changed the title Adds creation of a context to code execution under gno run. fix: Adds creation of a context to code execution under gno run. Sep 26, 2024
gnovm/cmd/gno/run_test.go Outdated Show resolved Hide resolved
gnovm/cmd/gno/run.go Outdated Show resolved Hide resolved
gnovm/cmd/gno/run_test.go Outdated Show resolved Hide resolved
@thehowl thehowl changed the title fix: Adds creation of a context to code execution under gno run. fix(cmd/gno): pass an ExecContext to MachineOptions in gno run Sep 26, 2024
@wyhaines wyhaines merged commit 14d4d21 into gnolang:master Sep 27, 2024
119 of 120 checks passed
@@ -79,6 +79,10 @@ func TestRunApp(t *testing.T) {
args: []string{"run", "../../tests/integ/invalid_assign/main.gno"},
recoverShouldContain: "cannot use bool as main.C without explicit conversion",
},
{
args: []string{"run", "-expr", "Context()", "../../tests/integ/context/context.gno"},
stdoutShouldContain: "Context worked",
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to have the height so we can detect if it changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

gno run should have an ExecContext
3 participants