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(stdlibs/std)!: change TestSetPrevRealm to TestSetRealm #2164

Merged
merged 5 commits into from
May 29, 2024

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented May 22, 2024

This PR removes the TestSetPrevRealm and TestSetPrevAddr, added in #890, in favour of a new API TestSetRealm, which is more flexible in its behaviour.

The function works by setting the result of CurrentRealm() for the frame in which the TestSetRealm function is called. Consequently, calling PrevRealm in any function called within will yield the value passed to TestSetRealm.

TestSetRealm allows to transparently set "user" and "code" realms, by using two new costructors in the testing stdlibs, std.NewUserRealm and std.NewCodeRealm, allowing emulation of both kinds of callers.

The call is not TestSetPrevRealm as it does not permanently "set" the value returned by PrevRealm, as if in a mock; it simply changes the Realm of the current frame, allowing "deep" calls to PrevRealm() to correctly return the value of the previous realm.

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.

@thehowl thehowl self-assigned this May 22, 2024
@thehowl thehowl requested review from jaekwon, moul, piux2, deelawn, mvertes and a team as code owners May 22, 2024 09:46
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label May 22, 2024
@thehowl thehowl requested a review from a team as a code owner May 22, 2024 13:06
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label May 22, 2024
@thehowl
Copy link
Member Author

thehowl commented May 22, 2024

cc @r3v4s

this will change the API of #891, but it's for the better :)

Copy link

codecov bot commented May 22, 2024

Codecov Report

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

Project coverage is 49.16%. Comparing base (a548238) to head (8de82d0).
Report is 1 commits behind head on master.

Files Patch % Lines
gnovm/pkg/gnolang/machine.go 0.00% 8 Missing ⚠️
gnovm/stdlibs/std/native.go 12.50% 6 Missing and 1 partial ⚠️
gnovm/stdlibs/std/banker.go 0.00% 5 Missing ⚠️
gnovm/tests/stdlibs/std/std.go 86.84% 5 Missing ⚠️
gnovm/stdlibs/stdlibs.go 0.00% 2 Missing ⚠️
gnovm/tests/imports.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2164      +/-   ##
==========================================
+ Coverage   49.12%   49.16%   +0.04%     
==========================================
  Files         576      577       +1     
  Lines       77805    77583     -222     
==========================================
- Hits        38219    38145      -74     
+ Misses      36495    36358     -137     
+ Partials     3091     3080      -11     
Flag Coverage Δ
gnovm 42.35% <71.42%> (+0.15%) ⬆️

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 May 24, 2024

I like the new API that takes a struct and fixes the expected behavior. However, I'm worried about the name. Shouldn't std.TestSetPrevRealm(rlm) change the behavior of std.PrevRealm() rlm?

@moul moul mentioned this pull request May 24, 2024
3 tasks
@thehowl
Copy link
Member Author

thehowl commented May 24, 2024

I like the new API that takes a struct and fixes the expected behavior. However, I'm worried about the name. Shouldn't std.TestSetPrevRealm(rlm) change the behavior of std.PrevRealm() rlm?

Updated OP to better describe reasoning

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.

Im still not super familiar with this API, but it's looks great too me 👍

@thehowl thehowl merged commit 380d7bf into master May 29, 2024
46 checks passed
@thehowl thehowl deleted the dev/morgan/better-setprevrealm branch May 29, 2024 10:56
omarsy pushed a commit to TERITORI/gno that referenced this pull request Jun 3, 2024
…2164)

This PR removes the `TestSetPrevRealm` and `TestSetPrevAddr`, added in
gnolang#890, in favour of a new API `TestSetRealm`, which is more flexible in
its behaviour.

The function works by setting the result of `CurrentRealm()` for the
frame in which the `TestSetRealm` function is called. Consequently,
calling `PrevRealm` in any function called within will yield the value
passed to `TestSetRealm`.

`TestSetRealm` allows to transparently set "user" and "code" realms, by
using two new costructors in the testing stdlibs, `std.NewUserRealm` and
`std.NewCodeRealm`, allowing emulation of both kinds of callers.

The call is not `TestSetPrevRealm` as it does not permanently "set" the
value returned by PrevRealm, as if in a mock; it simply changes the
Realm of the current frame, allowing "deep" calls to PrevRealm() to
correctly return the value of the previous realm.


<details><summary>Contributors' checklist...</summary>

- [ ] 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](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
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 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants