Hi @adriantam , I've noticed that in .golangci.yaml the min-complexity is currently set to 58.

Checking whole project, top 3 worst cyclomatic complexity scores are

I'm curious if it would be reasonable for the team to reduce this number or if there are reasons for keeping it that way. Thanks for sharing.

Hi @adriantam , I've noticed that in .golangci.yaml the min-complexity is currently set to 58.

Checking whole project, top 3 worst cyclomatic complexity scores are

PkgName FuncName Score
run (*ServerContext).Run 49 (expected to go down to 26)
config (*Config).Verify 43
graph (*RelationshipGraph).getRelationshipEdgesWithTargetRewrite 38
I'm curious if it would be reasonable for the team to reduce this number or if there are reasons for keeping it that way. Thanks for sharing.

Yes, we should reduce the allowed complexity going forward

Codecov Report

Attention: Patch coverage is 41.86047% with 75 lines in your changes are missing coverage. Please review.

Project coverage is 86.26%. Comparing base (aa623fb) to head (29d1135).

❗ Current head 29d1135 differs from pull request most recent head 655edfd. Consider uploading reports for the commit 655edfd to get more accurate results

@@            Coverage Diff             @@
##             main    #1605      +/-   ##
==========================================
- Coverage   86.39%   86.26%   -0.12%     
==========================================
  Files          86       86              
  Lines        8224     8244      +20     
==========================================
+ Hits         7104     7111       +7     
- Misses        789      798       +9     
- Partials      331      335       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Heads-up to reviewers! This change is a tiny bit risky. We have no test coverage for the playground_enabled flag: https://app.codecov.io/gh/openfga/openfga/pull/1605/blob/cmd/run/run.go. We should do some manual testing before merging.

HI @adriantam ,
in order to improve code coverage I can add some unit tests to cover playground and profiler related paths in code

// sample unit test code

func TestProfilerEnabled(t *testing.T) {
	cfg := testutils.MustDefaultConfigWithRandomPorts()
	cfg.Profiler.Enabled = true

	ctx, cancel := context.WithCancel(context.Background())
	defer cancel()

	go func() {
		if err := runServer(ctx, cfg); err != nil {
			log.Fatal(err)
		}
	}()

	profilerAddr := fmt.Sprintf("localhost%s", cfg.Profiler.Addr)
	profilerPaths := []string{
		"/debug/pprof/",
		"/debug/pprof/cmdline",
		"/debug/pprof/profile",
		"/debug/pprof/symbol",
		"/debug/pprof/trace",
	}
	var resp *http.Response
	var err error
	for i := 0; i< len(profilerPaths); i++ {
		resp, err = retryablehttp.Get(fmt.Sprintf("http://%s%s", profilerAddr, profilerPaths[i]))
		require.NoErrorf(t, err, "http profiler endpoint %s not healthy", profilerPaths[i])
		require.Equal(t, http.StatusOK, resp.StatusCode, "unexpected status code received from server")
	}

	t.Cleanup(func() {
		err := resp.Body.Close()
		require.NoError(t, err)
	})
}

func TestPlaygroundEnabled(t *testing.T) {
	cfg := testutils.MustDefaultConfigWithRandomPorts()
	cfg.Playground.Enabled = true

	ctx, cancel := context.WithCancel(context.Background())
	defer cancel()

	go func() {
		if err := runServer(ctx, cfg); err != nil {
			log.Fatal(err)
		}
	}()

	// testutils.EnsureServiceHealthy(t, cfg.GRPC.Addr, cfg.HTTP.Addr, nil)
	playgroundPort := fmt.Sprintf(":%d", cfg.Playground.Port)
	resp, err := retryablehttp.Get(fmt.Sprintf("http://localhost%s/playground", playgroundPort))
	require.NoError(t, err, "http playground endpoint not healthy")
	
	for i:=0; i<3 && resp.StatusCode == http.StatusNotFound; i++ {
		resp, err = retryablehttp.Get(fmt.Sprintf("http://localhost%s/playground", playgroundPort))
		require.NoError(t, err, "http playground endpoint not healthy")
	}

	t.Cleanup(func() {
		err := resp.Body.Close()
		require.NoError(t, err)
	})
	require.Equal(t, http.StatusOK, resp.StatusCode, "unexpected status code received from server")
}

But I have several questions regarding the approach:

1. there's duplicated logic mostly copied from testutils.EnsureServiceHealthy. I was wondering if worth it to create testutils.EnsureServicePathHealthy(t, cfg.HTTP.Addr, pathsToCheck)

2. At least in my computer the new test cases make the whole package test take 62 secs, compared to the 27 secs it took before. Most delay to blame on profiler routes (eg: /debug/pprof/profile).
   Is there any timeout limit while running Git workflow checks?

3. Finally, the proposed changes move my local coverage from 80.0% to 88.5%
   I'm not sure how to calculate "codecov/patch" locally. Any help you can share?

Thanks for your support

Did more research to self answer my questions

1. there's duplicated logic mostly copied from testutils.EnsureServiceHealthy. I was wondering if worth it to create testutils.EnsureServicePathHealthy(t, cfg.HTTP.Addr, pathsToCheck)

Only adding test for playground refactor to share code and make it easier to review.
With this change, package coverage jumps from 80.0% to 86.3%

2. At least in my computer the new test cases make the whole package test take 62 secs, compared to the 27 secs it took before. Most delay to blame on profiler routes (eg: /debug/pprof/profile).
   Is there any timeout limit while running Git workflow checks?

I found that timeout should be 15mins based on:

3. Finally, the proposed changes move my local coverage from 80.0% to 88.5%
   I'm not sure how to calculate "codecov/patch" locally. Any help you can share?

I will push to see the outcome and avoid wasting other people time. If there's an easy way to share, happy to know it.

OpenFGA Team,
Note that the merge was really messy to compare. I've reset local branch to match upstream/main and will re-submit the changes for review