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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃 [Question]: app.Test may hang infinitely with handler calling runtime.Goexit #1996

Closed
3 tasks done
trim21 opened this issue Aug 6, 2022 · 5 comments 路 Fixed by #1997
Closed
3 tasks done

馃 [Question]: app.Test may hang infinitely with handler calling runtime.Goexit #1996

trim21 opened this issue Aug 6, 2022 · 5 comments 路 Fixed by #1997

Comments

@trim21
Copy link
Contributor

trim21 commented Aug 6, 2022

Question Description

I'm using github.com/stretchr/testify/mock to create a mock, and this library may call t.FailNow() if caller try to do a unexpected method call.

But (*testing.T).FailNow() will only call runtime.Goexit but not panic.

Therefore, the handler can't panic if the test fail. and I can only get a timeout error instead of panic error.

and, if app.Test is called without timeout, this test will hang infinitely .

I want to know is there a way to handle this problem?

Code Snippet (optional)

package main_test

import (
	"net/http"
	"net/http/httptest"
	"testing"

	"github.com/gofiber/fiber/v2"
)

func TestFailNow(t *testing.T) {
	app := fiber.New()

	app.Get("/", func(c *fiber.Ctx) error {
		t.FailNow() // or runtime.Goexit()
		return nil
	})

	req := httptest.NewRequest(http.MethodGet, "/", http.NoBody)

	res, err := app.Test(req, -1)
	if err != nil {
		t.Error(err)
		t.FailNow()
	}

	if res.StatusCode != 502 {
		t.FailNow()
	}
}

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have checked for existing issues that describe my questions prior to opening this one.
  • I understand that improperly formatted questions may be closed without explanation.
@trim21

This comment was marked as outdated.

@wangjq4214
Copy link
Member

runtime.Goexit() will call all unexecuted defer functions and exit the current goroutine, causing the error channel in Test function to not receive any messages.

And I think it's important to check the parameters at the controller level and make sure that the mock covers these test cases, rather than making the mock function call t.FailNow.

Changing app.Test will introduce unnecessary complexity.

@trim21
Copy link
Contributor Author

trim21 commented Aug 6, 2022

this is more a developer experience problem.

Mocks may call t.FailNow when it can't find matched called, usually happens when writing tests and mock are not properly set. t.FailNow is not a expected behavior.

if a test disable app.Test timeout, this may cause a test hang very long time until go.test reach default go test timeout and kill all test

@trim21
Copy link
Contributor Author

trim21 commented Aug 6, 2022

After reading source of app.Test, I think it's not too complex to handle this, just add a defer in the goroutine start by app.Test

	go func() {
		channel <- app.server.ServeConn(conn)
	}()
	go func() {
		var returned bool
		defer func() {
			if !returned {
				channel <- fmt.Errorf("runtime.Goexit() called in handler")
			}
		}()

		r := app.server.ServeConn(conn)
		returned = true
		channel <- r
	}()

@trim21 trim21 changed the title 馃 [Question]: test with mock may call t.FailNow() 馃 [Question]: app.Test may hang infinitely with handler calling runtime.Goexit Aug 6, 2022
@wangjq4214
Copy link
Member

this is more a developer experience question.

Mocks may call t.FailNow when it can't find matched called, usually happens when writing tests and mock are not properly set. t.FailNow is not a expected behavior.

if a test disable app.Test timeout, this may cause a test hang very long time until go.test reach default go test timeout and kill all test

I get your mean, the long hang-up is indeed unexpected behavior.

@ReneWerner87 ReneWerner87 linked a pull request Aug 8, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants