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: Fixes #2938 GnoVM Unexpected Crash When Panic Occurs in Tests #2942

Closed

Conversation

salihdhaifullah
Copy link

@salihdhaifullah salihdhaifullah commented Oct 12, 2024

fixed panic crash on gno test by correctly formatting the argument for m.Panic

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.

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Oct 12, 2024
@salihdhaifullah salihdhaifullah changed the title Fixes #2938 GnoVM Unexpected Crash When Panic Occurs in Tests fix: Fixes #2938 GnoVM Unexpected Crash When Panic Occurs in Tests Oct 12, 2024
@notJoon notJoon added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 14, 2024
@notJoon
Copy link
Member

notJoon commented Oct 14, 2024

Thank you for contributing. Could you please add some test for this?

@salihdhaifullah
Copy link
Author

I have added a test file i hope i did that right

Copy link
Member

@notJoon notJoon left a comment

Choose a reason for hiding this comment

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

The modified lines seem fine and the tests are running well.

Result

$ cd gnovm/cmd/gno 
$ go test -v . -run Test_Scripts/gno_test/panic_error

=== RUN   Test_Scripts
    testdata_test.go:27: testing: gno_fmt
    testdata_test.go:27: testing: gno_lint
    testdata_test.go:27: testing: gno_test
=== RUN   Test_Scripts/gno_test
=== RUN   Test_Scripts/gno_test/panic_error
=== PAUSE Test_Scripts/gno_test/panic_error
=== CONT  Test_Scripts/gno_test/panic_error
    testscript.go:558: WORK=$WORK
        PATH=/* ... */
        GOTRACEBACK=system
        HOME=/var/folders/sb/8c04v17n44zd7x__zvgs2vz00000gn/T/gno3283009104
        TMPDIR=$WORK/.tmp
        devnull=/dev/null
        /=/
        :=:
        $=$
        exe=
        GNOROOT=/Users/notJoon/gno-core
        GO111MODULE=off
        
        # Test panic with error output in a test (1.069s)
        > ! gno test .
        [stderr]
        --- WARNING: unable to read package path from gno.mod or gno root directory; try creating a gno.mod file
        panic: hello world
        --- FAIL: TestPanicWithError (0.00s)
        .: test pkg: failed: "TestPanicWithError"
        FAIL
        FAIL    .       0.65s
        FAIL
        FAIL
        FAIL: 0 build errors, 1 test errors
        
        gno command error: exit status 1
        > ! stdout .+
        > stderr '--- FAIL: TestPanicWithError'
        > stderr 'panic: hello world'
        > stderr 'FAIL'
        PASS

remove review/triage-pending flag due to the above reason

@notJoon notJoon removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 22, 2024
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. This PR is not a good way to approach the problem, as it's changing the behaviour of how we panic, by special-casing in the GnoVM what happens from the panic message. This is bad.

What we should do instead, is that we can understand error values in gno2go, and handle them accordingly, ie. converting them to Go errors, so that the call to functions like Printf work properly. This is not ideal either, but preferable since gno2go code only concerns testing and there is a long-term plan to remove it: #1361

I'm closing this PR; feel free to make a new one with a new approach.

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.

3 participants