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

#931 breaks re-running lifecycle hooks #945

Open
abhinav opened this issue Sep 28, 2022 · 5 comments
Open

#931 breaks re-running lifecycle hooks #945

abhinav opened this issue Sep 28, 2022 · 5 comments

Comments

@abhinav
Copy link
Collaborator

abhinav commented Sep 28, 2022

In #931, we added checks to ensure that lifecycle hooks only run once to avoid users accidentally running app.Stop twice by doing: app.Run, app.Stop.

This seems to have broken some users like @zenthangplus, who are re-running app.Start per-request.


    @sywhang In my case, i've wrote test for integration api testing. every http call to my api will be handled by this function.
httpHandler := func(w http.ResponseWriter, r *http.Request) {
		ctx := context.Background()
		ctxWithResponseWriter := context.WithValue(ctx, "w", w)
		ctxWithRequest := context.WithValue(ctxWithResponseWriter, "r", r)
		_ = app.Start(ctxWithRequest)
	}

Before it works well, but after this MR #931 app.Start is only allow to run once. I can change my logic to handle app.Stop before handle another request but i think this is breaking change and it break my test logic.

(again, it not related to this MR)

Originally posted by @zenthangplus in #941 (comment)

@zenthangplus
Copy link

@abhinav I realize that app start per-request is a pretty common approach in Golang Test

@abhinav
Copy link
Collaborator Author

abhinav commented Sep 30, 2022

@zenthangplus Apologies, I didn't mean that to suggest you were doing anything incorrectly.
It's just that that is not how we expected fx.App to be used. We expected it to Start only once.
In our tests internally, we usually use fxtest.New to create a new App per test case and throw it away afterwards. (It may handle multiple requests.)

To facilitate this, our service scaffolding system generates something like this:

func App() fx.Option {
  return fx.Options(
    fx.Provide(...),
   // ...
  )
}

func main() {
  fx.New(App()).Run()
}

And then inside tests, we do,

app := fxtest.New(t, App())
defer app.RequireStart().RequireStop()

However, I realize others may not be following this pattern.

Would you mind sharing more about how you're structuring your tests and calling App.Start repeatedly so we can better understand this use case?

@zenthangplus
Copy link

zenthangplus commented Oct 1, 2022

@abhinav i'm discussing about integration test for rest api service (i've other case for worker service, more complexity, we will discuss later). about your example, how do you send test request to your api service app. I guess that you need to know the endpoint of your test app (eg: http://localhost:8080/...).

In my case, i don't want to send test request over network because we have so many tests, and it will take too long time. So my solution is using a mocked http client to trigger app.Start instead of send test request over network.

Here is how that solution work:

Inside main app:

func App() fx.Option {
  return fx.Options(
    fx.Provide(...),
  )
}

func main() {
  fx.New(App()).Run()
}

Then inside test app, i initialize fx.App once for all tests.

var app *fx.App

// i want initialize `fx.App` once for all tests.
// I'm not using `fxtest.New` because i don't have testing.TB here
function init() {
   app = fx.New(App(), fx.Invoke(func(lc fx.Lifecycle, httpServer *http.Server) {
  	lc.Append(
  		fx.Hook{
			OnStart: func(ctx context.Context) error {
				r := ctx.Value("r").(*http.Request)
				w := ctx.Value("w").(*httptest.ResponseRecorder)
				httpServer.Handler.ServeHTTP(w, r)
			},
		},
	)
   }))
}

var httpHandler = func(w http.ResponseWriter, r *http.Request) {
	ctx := context.Background()
	ctxWithResponseWriter := context.WithValue(ctx, "w", w)
	ctxWithRequest := context.WithValue(ctxWithResponseWriter, "r", r)
	_ = app.Start(ctxWithRequest)
}

Then inside every tests i will using mocked http client to call http request to test app:

func TestExample(t *testing.T) {
   request := .. build http.Request...
   responseRecorder = httptest.NewRecorder()
   httpHandler(responseRecorder, request)
}

@jkanywhere
Copy link
Contributor

jkanywhere commented Oct 6, 2022

how do you send test request to your api service app. I guess that you need to know the endpoint of your test app (eg: http://localhost:8080/...).

Exactly. The test starts the app listening on a local port and we send requests to the local port. The test server and client need to agree on the port.
If you use a static port, it only allows one instance of the test running at a time. Works well for local development, but poorly on CI with hundreds or thousands of tests.

You can start the server on a dynamic port, use fx.Populate to extract that information from the running application, and tell the client what port was chosen.

It could look something like this:

func TestExample(t *testing.T) {
    var listener net.Listener
    app := fxtest.New(t, fx.Options(
        App(),
        fx.Provide(func() (net.Listener, error) (
            return net.Listen("tcp4", "127.0.0.1:0")
        }),
        fx.Invoke(func(Server, net.Listener) {
            go server.Serve(listener)
        }),
        fx.Populate(&listener),
    )
    defer app.RequireStart().RequireStop()

    require.NotNil(t, listener, "unable to find listening port for server")

    // Now you can use the listener address to construct a client object...
    client := newClient(listener.Addr())
    client.MakeRequest(context.Background())

    // ...or construct an http request.
    request := http.NewRequest(http.MethodPost, listener.Addr(), ...)

}

fx.Provide...net.Listener and fx.Invoke...server.Serve are written here in the test function explicitly, but they could be part of your App() already/instead.

@zenthangplus
Copy link

Exactly. The test starts the app listening on a local port and we send requests to the local port. The test server and client need to agree on the port.
If you use a static port, it only allows one instance of the test running at a time. Works well for local development, but poorly on CI with hundreds or thousands of tests.

I know this, but this is my mean:

In my case, i don't want to send test request over network because we have so many tests, and it will take too long time. So my solution is using a mocked http client to trigger app.Start instead of send test request over network.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants