Skip to content

testing: implement {Skip,Fail}Now #4736

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

Merged
merged 1 commit into from
Feb 15, 2025

Conversation

leongross
Copy link
Contributor

PR #4623 implements runtime.GoExit() with which we can improve the testing package and align it more to upstream testing behavior https://cs.opensource.google/go/go/+/refs/tags/go1.24.0:src/testing/testing.go;l=1150

@leongross leongross self-assigned this Feb 14, 2025
@leongross leongross changed the title testing: implement {SKip,Fail}Now testing: implement {Skip,Fail}Now Feb 14, 2025
PR tinygo-org#4623 implements
runtime.GoExit() with which we can improve the testing package
and align it more to upstream testing behavour https://cs.opensource.google/go/go/+/refs/tags/go1.24.0:src/testing/testing.go;l=1150

Signed-off-by: leongross <[email protected]>
@deadprogram
Copy link
Member

Thanks for the improvement @leongross now merging.

@deadprogram deadprogram merged commit c5879c6 into tinygo-org:dev Feb 15, 2025
18 checks passed
@leongross
Copy link
Contributor Author

@deadprogram thanks for reviewing and merging.

@leongross leongross deleted the testing/SkipNow branch February 15, 2025 21:43
leongross added a commit to leongross/u-root that referenced this pull request Feb 16, 2025
PR tinygo-org/tinygo#4736 adds runtime.Goexit to {Skip,Fail}Now,
which currently fails in the go tests.

Signed-off-by: leongross <[email protected]>
leongross added a commit to leongross/u-root that referenced this pull request Feb 17, 2025
PR tinygo-org/tinygo#4736 adds runtime.Goexit to {Skip,Fail}Now,
which currently fails in the go tests.

Signed-off-by: leongross <[email protected]>
leongross added a commit to leongross/u-root that referenced this pull request Feb 17, 2025
PR tinygo-org/tinygo#4736 adds runtime.Goexit to {Skip,Fail}Now,
which currently fails in the go tests.

Signed-off-by: leongross <[email protected]>
leongross added a commit to leongross/u-root that referenced this pull request Feb 19, 2025
PR tinygo-org/tinygo#4736 adds runtime.Goexit to {Skip,Fail}Now,
which currently fails in the go tests.

Signed-off-by: leongross <[email protected]>
@aykevl aykevl mentioned this pull request Feb 24, 2025
@aykevl
Copy link
Member

aykevl commented Feb 24, 2025

@leongross how did you test this? Because when I try to run a trivial test like this:

package main_test

import "testing"

func TestFoo(t *testing.T) {
        t.Skip("testing skip")
        println("after skip")
}

It crashes with an error like this:

$ GOARCH=amd64 build/tinygo test -c -o test.exe ./tmp/test
$ ./test.exe
panic: runtime error at 0x00000000004062e7: deadlocked: no event source

(Currently testing in a Windows ARM VM, I can later test the same thing on Linux for example).

@aykevl
Copy link
Member

aykevl commented Feb 24, 2025

This is also happening on MacOS:

$ tinygo test -v ./tmp/test
=== RUN   TestFoo
    testing skip
panic: runtime error at 0x00000001029ddf24: deadlocked: no event source
FAIL	github.com/tinygo-org/tinygo/tmp/test	0.186s

aykevl added a commit that referenced this pull request Feb 25, 2025
This reverts commit c5879c6.

It doesn't look like this is working (see
#4736 (comment)),
and it doesn't have any tests anyway to prove that it does work. So I
think it's best to revert it for now and add a working implementation
with tests in the future.
deadprogram pushed a commit that referenced this pull request Feb 25, 2025
This reverts commit c5879c6.

It doesn't look like this is working (see
#4736 (comment)),
and it doesn't have any tests anyway to prove that it does work. So I
think it's best to revert it for now and add a working implementation
with tests in the future.
@leongross
Copy link
Contributor Author

@aykevl I investigated this error and found that it has to do with the signal handling in the runtime_unix.go in the waitForEvents function. If hasSignals is false, this deadlock occurs. Iirc, this variable is set to true when we enable a single using the signal_enable function. When the signal_enable function is not called, the hasSignals variable is false by default, and then this panic occurs.

The waitForSignals function is called in the scheduler function where it then panics on the first call to it.

@aykevl
Copy link
Member

aykevl commented Feb 27, 2025

@leongross that's unrelated. The problem is that it exits the main goroutine (at least that's what I think happens) so there's no goroutine left. The reason it deadlocks instead of emitting a runtime panic when a signal is enabled, is because theoretically a signal might arrive which might unblock a blocked goroutine. But then you just end up with a deadlocked program instead of a runtime panic - not an improvement.

I think the only way to implement Skip etc is by running each test in a separate goroutine, which would mean runtime.Goexit() indeed exits just that test instead of the whole program. But starting a new goroutine for each test is expensive, especially on embedded. So there's a tradeoff here.

In any case, next time please include a test to show that it works :)
There are a bunch of tests in testdata/testing.go where a t.Skip() test could be added.

jensdrenhaus pushed a commit to leongross/u-root that referenced this pull request Mar 20, 2025
PR tinygo-org/tinygo#4736 adds runtime.Goexit to {Skip,Fail}Now,
which currently fails in the go tests.

Signed-off-by: leongross <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants