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!: remove flag -transpile from gno test #2050

Merged
merged 2 commits into from
May 30, 2024

Conversation

harry-hov
Copy link
Contributor

@harry-hov harry-hov commented May 7, 2024

The -transpile flag in the gno test command duplicates the functionality of the standalone gno transpile command. Removed the unnecessary -transpile flag from the gno test command.

gno test should only be focused on testing!

@harry-hov harry-hov added the breaking change Functionality that contains breaking changes label May 7, 2024
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label May 7, 2024
Copy link

codecov bot commented May 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.98%. Comparing base (0d299c3) to head (ab90f24).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2050      +/-   ##
==========================================
+ Coverage   49.96%   49.98%   +0.02%     
==========================================
  Files         578      578              
  Lines       77864    77808      -56     
==========================================
- Hits        38903    38896       -7     
+ Misses      35839    35790      -49     
  Partials     3122     3122              
Flag Coverage Δ
gnovm 45.16% <ø> (+0.05%) ⬆️

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.

@harry-hov harry-hov marked this pull request as ready for review May 7, 2024 14:47
@thehowl
Copy link
Member

thehowl commented May 7, 2024

cc @moul wdyt?

it seems that transpiling mostly served the purpose of attempting to build the transpiled go source; ie. static checking. though, effectively, this is moved to gno lint in #1730 . I tend to agree with @harry-hov to separate the concerns

@moul
Copy link
Member

moul commented May 29, 2024

My goal with this flag was to have « gno test » exiting 0 of everything is OK and 1 of anything is bad.

‘before this flag, we basically had to run two commands to « test ».

ideally we should remove the flag but have a stronger type checking.

as soon as we’re testing to transpire on the CI, I’m fine removing the flag.

@harry-hov
Copy link
Contributor Author

ideally we should remove the flag but have a stronger type checking.

I agree. I think @thehowl is already working on this!

as soon as we’re testing to transpire on the CI, I’m fine removing the flag.

We already have workflow for transpiling /examples

- run: go run ./gnovm/cmd/gno transpile -v --gobuild ./examples

Infact we never run gno test with -transpile flag on CI.

@moul
Copy link
Member

moul commented May 29, 2024

Let's remove the flag. Let's keep transpile in the CI and wait for stronger type checking to make gno test testing everything, as it should.

@thehowl
Copy link
Member

thehowl commented May 30, 2024

We'll add type checking in gno test with #1730

@thehowl thehowl merged commit 705f0e0 into gnolang:master May 30, 2024
46 checks passed
omarsy pushed a commit to TERITORI/gno that referenced this pull request Jun 3, 2024
The `-transpile` flag in the `gno test` command duplicates the
functionality of the standalone `gno transpile` command. Removed the
unnecessary `-transpile` flag from the `gno test` command.

`gno test` should only be focused on testing!

Co-authored-by: Morgan <morgan@morganbaz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Functionality that contains breaking changes 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants