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

Merging to release-5.2: [TT-13819] Benchmark updates, session limiter workaround for test goroutine leak (#6826) #6834

Conversation

buger
Copy link
Member

@buger buger commented Jan 16, 2025

User description

TT-13819 Benchmark updates, session limiter workaround for test goroutine leak (#6826)

PR Type

Enhancement, Tests


Description

  • Added DisableRateLimit and DisableQuota flags in multiple test
    cases.

  • Improved benchmark scripts and added skipping for specific benchmarks.

  • Refactored session limiter to encourage reuse of bucketStore.

  • Updated CI benchmark script for better profiling and output.


Changes walkthrough 📝

Relevant files
Tests
7 files
api_definition_test.go
Added DisableRateLimit and DisableQuota flags in API definition tests
+2/-0     
event_system_test.go
Added benchmark skipping for generic event handlers           
+2/-0     
mw_basic_auth_test.go
Added `DisableRateLimit` and `DisableQuota` flags in basic auth tests
+2/-1     
mw_jwt_test.go
Added `DisableRateLimit` and `DisableQuota` flags in JWT tests
+10/-0   
mw_transform_test.go
Refactored transform middleware benchmarks for better structure
+9/-7     
oauth_manager_test.go
Added benchmark skipping for OAuth token purging                 
+2/-0     
res_handler_header_injector_test.go
Adjusted header injection benchmarks and test cases           
+7/-6     
Enhancement
2 files
session_manager.go
Refactored session limiter to reuse `bucketStore`               
+4/-1     
ci-benchmark.sh
Enhanced CI benchmark script with profiling and output improvements
+9/-3     
Configuration changes
1 files
CODEOWNERS
Updated ownership for `/bin/` directory                                   
+1/-0     

💡 PR-Agent usage: Comment /help "your question" on any pull
request to receive relevant information


Co-authored-by: Tit Petric tit@tyk.io


PR Type

Enhancement, Tests


Description

  • Introduced DisableRateLimit and DisableQuota flags across multiple test cases.

  • Enhanced benchmark scripts with skipping and profiling improvements.

  • Refactored session limiter to reuse bucketStore for efficiency.

  • Updated CI benchmark script for structured profiling and output.


Changes walkthrough 📝

Relevant files
Tests
8 files
api_definition_test.go
Added `DisableRateLimit` and `DisableQuota` flags in API tests
+2/-0     
event_system_test.go
Added benchmark skipping for generic event handlers           
+2/-0     
mw_basic_auth_test.go
Added `DisableRateLimit` and `DisableQuota` flags in basic auth tests
+2/-1     
mw_jwt_test.go
Added `DisableRateLimit` and `DisableQuota` flags in JWT tests
+10/-0   
mw_transform_test.go
Refactored transform middleware benchmarks for better structure
+58/-1   
mw_version_check_test.go
Added `DisableRateLimit` and `DisableQuota` flags in versioning tests
+2/-0     
oauth_manager_test.go
Added benchmark skipping and new tests for OAuth token purging
+91/-0   
res_handler_header_injector_test.go
Adjusted header injection benchmarks and test cases           
+7/-6     
Enhancement
2 files
session_manager.go
Refactored session limiter to reuse `bucketStore`               
+22/-0   
ci-benchmark.sh
Enhanced CI benchmark script with profiling improvements 
+13/-3   
Configuration changes
1 files
CODEOWNERS
Updated ownership for `/bin/` directory and workflows       
+15/-0   

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

…outine leak (#6826)

Enhancement, Tests

___

- Added `DisableRateLimit` and `DisableQuota` flags in multiple test
cases.

- Improved benchmark scripts and added skipping for specific benchmarks.

- Refactored session limiter to encourage reuse of `bucketStore`.

- Updated CI benchmark script for better profiling and output.

___

<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Tests</strong></td><td><details><summary>7
files</summary><table>
<tr>
<td><strong>api_definition_test.go</strong><dd><code>Added
<code>DisableRateLimit</code> and <code>DisableQuota</code> flags in API
definition tests</code></dd></td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6826/files#diff-2394daab6fdc5f8dc234699c80c0548947ee3d68d2e33858258d73a8b5eb6f44">+2/-0</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>

<tr>
<td><strong>event_system_test.go</strong><dd><code>Added benchmark
skipping for generic event handlers</code>&nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; </dd></td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6826/files#diff-59fda3ebcb87e3c16f222e51988dfb18be9239c84caeef0137db04ffe9ab8f3c">+2/-0</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>

<tr>
<td><strong>mw_basic_auth_test.go</strong><dd><code>Added
`DisableRateLimit` and `DisableQuota` flags in basic auth
tests</code></dd></td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6826/files#diff-148b4d4c5c77fa4382e83fd583c36d21bba7b52217f821095abbc517d277b16f">+2/-1</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>

<tr>
<td><strong>mw_jwt_test.go</strong><dd><code>Added `DisableRateLimit`
and `DisableQuota` flags in JWT tests</code></dd></td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6826/files#diff-406bf8fdb6c0cc77f04c6245c70abfc592ddb1525aa843200d850e14d135ebfc">+10/-0</a>&nbsp;
&nbsp; </td>

</tr>

<tr>
<td><strong>mw_transform_test.go</strong><dd><code>Refactored transform
middleware benchmarks for better structure</code></dd></td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6826/files#diff-ec6f80b3dcb001b2a2ac9b7277fff606bd77d796f99b8c1d10b8d0d55863deb3">+9/-7</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>

<tr>
<td><strong>oauth_manager_test.go</strong><dd><code>Added benchmark
skipping for OAuth token purging</code>&nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6826/files#diff-139c3beb238f234728bde9f619f501cf730a7e275d17c8511277272d1dcd6fc6">+2/-0</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>

<tr>

<td><strong>res_handler_header_injector_test.go</strong><dd><code>Adjusted
header injection benchmarks and test cases</code>&nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; </dd></td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6826/files#diff-1c82da1ac85593e31ce947ef821703967ceb8065a5434c6749b802a1b93f9117">+7/-6</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>

</table></details></td></tr><tr><td><strong>Enhancement</strong></td><td><details><summary>2
files</summary><table>
<tr>
<td><strong>session_manager.go</strong><dd><code>Refactored session
limiter to reuse `bucketStore`</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; </dd></td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6826/files#diff-e6b40a285464cd86736e970c4c0b320b44c75b18b363d38c200e9a9d36cdabb6">+4/-1</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>

<tr>
<td><strong>ci-benchmark.sh</strong><dd><code>Enhanced CI benchmark
script with profiling and output improvements</code></dd></td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6826/files#diff-e789974499a5ba77194df612e751bc3db20b73c389466c679e4e74521199dd48">+9/-3</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>
</table></details></td></tr><tr><td><strong>Configuration
changes</strong></td><td><details><summary>1 files</summary><table>
<tr>
<td><strong>CODEOWNERS</strong><dd><code>Updated ownership for `/bin/`
directory</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
</dd></td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6826/files#diff-3d36a1bf06148bc6ba1ce2ed3d19de32ea708d955fed212c0d27c536f0bd4da7">+1/-0</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>
</table></details></td></tr></tr></tbody></table>

___

> 💡 **PR-Agent usage**: Comment `/help "your question"` on any pull
request to receive relevant information

---------

Co-authored-by: Tit Petric <tit@tyk.io>

(cherry picked from commit 155e11b)
Copy link
Contributor

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

Copy link
Contributor

github-actions bot commented Jan 16, 2025

API Changes

no api changes detected

Copy link
Contributor

github-actions bot commented Jan 16, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

6826 - Fully compliant

Fully compliant requirements:

  • Add DisableRateLimit and DisableQuota flags in multiple test cases.
  • Improve benchmark scripts and add skipping for specific benchmarks.
  • Refactor session limiter to encourage reuse of bucketStore.
  • Update CI benchmark script for better profiling and output.

Not compliant requirements:
None

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Merge Conflict Artifacts

The file contains unresolved merge conflict artifacts (e.g., <<<<<<< HEAD, =======, >>>>>>>). These should be resolved before merging.

<<<<<<< HEAD
	ad := apidef.APIDefinition{}
	spec := APISpec{APIDefinition: &ad}
	spec.EnableContextVars = false
	base := BaseMiddleware{Spec: &spec, Gw: ts.Gw}
	base.Spec.EnableContextVars = false
=======
	ad := &apidef.APIDefinition{}
	spec := &APISpec{APIDefinition: ad}
	base := &BaseMiddleware{Spec: spec, Gw: ts.Gw}

>>>>>>> 155e11bb3... [TT-13819] Benchmark updates, session limiter workaround for test goroutine leak (#6826)
	transform := TransformMiddleware{base}

	if err := transformBody(r, tmeta, &transform); err != nil {
		t.Fatalf("wanted nil error, got %v", err)
	}
	gotBs, err := ioutil.ReadAll(r.Body)
	if err != nil {
		t.Fatal(err)
	}
	if got := string(gotBs); got != want {
		t.Fatalf("wanted body %q, got %q", want, got)
	}
}

func BenchmarkTransformNonAscii(b *testing.B) {
	b.ReportAllocs()

	tmeta, in := testPrepareTransformNonAscii()

	ts := StartTest(nil)
	defer ts.Close()

<<<<<<< HEAD
	spec := APISpec{}
	base := BaseMiddleware{Spec: &spec, Gw: ts.Gw}
	base.Spec.EnableContextVars = false
	transform := TransformMiddleware{base}
=======
	ad := &apidef.APIDefinition{}
	spec := &APISpec{APIDefinition: ad}
	base := &BaseMiddleware{Spec: spec, Gw: ts.Gw}

	transform := &TransformMiddleware{base}
>>>>>>> 155e11bb3... [TT-13819] Benchmark updates, session limiter workaround for test goroutine leak (#6826)

	for i := 0; i < b.N; i++ {
		r := TestReq(b, "GET", "/", in)

		if err := transformBody(r, tmeta, transform); err != nil {
			b.Fatalf("wanted nil error, got %v", err)
		}
	}
}

func TestTransformXMLCrash(t *testing.T) {
	// mxj.NewMapXmlReader used to take forever and crash the
	// process by eating up all the memory.
	in := strings.NewReader("not xml")
	r := TestReq(t, "GET", "/", in)
	tmeta := &TransformSpec{}
	tmeta.TemplateData.Input = apidef.RequestXML
	tmeta.Template = textTemplate.Must(apidef.Template.New("").Parse(""))

	ts := StartTest(nil)
	defer ts.Close()

<<<<<<< HEAD
	ad := apidef.APIDefinition{}
	spec := APISpec{APIDefinition: &ad}
	base := BaseMiddleware{Spec: &spec, Gw: ts.Gw}
	base.Spec.EnableContextVars = false
=======
	ad := &apidef.APIDefinition{}
	spec := &APISpec{APIDefinition: ad}
	base := &BaseMiddleware{Spec: spec, Gw: ts.Gw}

>>>>>>> 155e11bb3... [TT-13819] Benchmark updates, session limiter workaround for test goroutine leak (#6826)
	transform := TransformMiddleware{base}

	if err := transformBody(r, tmeta, &transform); err == nil {
		t.Fatalf("wanted error, got nil")
	}
}

func testPrepareTransformJSONMarshal(inputType string) (tmeta *TransformSpec, in string) {
	tmeta = &TransformSpec{}
	tmpl := `[{{range $x, $s := .names.name}}{{$s | jsonMarshal}}{{if not $x}}, {{end}}{{end}}]`
	tmeta.TemplateData.Input = apidef.RequestXML
	tmeta.Template = textTemplate.Must(apidef.Template.New("").Parse(tmpl))

	switch inputType {
	case "json":
		tmeta.TemplateData.Input = apidef.RequestJSON
		in = `{"names": { "name": ["Foo\"oo", "Bàr"] }}`
	case "xml":
		tmeta.TemplateData.Input = apidef.RequestXML
		in = `<names>
	<name>Foo"oo</name>
	<name>Bàr</name>
</names>`
	}

	return tmeta, in
}

func testPrepareTransformXMLMarshal(tmpl string, inputType apidef.RequestInputType) (tmeta *TransformSpec) {
	tmeta = &TransformSpec{}
	tmeta.Template = textTemplate.Must(apidef.Template.New("").Parse(tmpl))

	switch inputType {
	case apidef.RequestJSON:
		tmeta.TemplateData.Input = apidef.RequestJSON
	case apidef.RequestXML:
		tmeta.TemplateData.Input = apidef.RequestXML
	}

	return tmeta
}

func TestTransformJSONMarshalXMLInput(t *testing.T) {
	tmeta, in := testPrepareTransformJSONMarshal("xml")

	want := `["Foo\"oo", "Bàr"]`
	r := TestReq(t, "GET", "/", in)

	ts := StartTest(nil)
	defer ts.Close()

<<<<<<< HEAD
	ad := apidef.APIDefinition{}
	spec := APISpec{APIDefinition: &ad}
	base := BaseMiddleware{Spec: &spec, Gw: ts.Gw}
	base.Spec.EnableContextVars = false
=======
	ad := &apidef.APIDefinition{}
	spec := &APISpec{APIDefinition: ad}
	base := &BaseMiddleware{Spec: spec, Gw: ts.Gw}

>>>>>>> 155e11bb3... [TT-13819] Benchmark updates, session limiter workaround for test goroutine leak (#6826)
	transform := TransformMiddleware{base}

	if err := transformBody(r, tmeta, &transform); err != nil {
		t.Fatalf("wanted nil error, got %v", err)
	}
	gotBs, err := ioutil.ReadAll(r.Body)
	if err != nil {
		t.Fatal(err)
	}
	if got := string(gotBs); got != want {
		t.Fatalf("wanted body %q, got %q", want, got)
	}
}

func TestTransformJSONMarshalJSONInput(t *testing.T) {
	tmeta, in := testPrepareTransformJSONMarshal("json")

	want := `["Foo\"oo", "Bàr"]`
	r := TestReq(t, "GET", "/", in)

	ts := StartTest(nil)
	defer ts.Close()

<<<<<<< HEAD
	ad := apidef.APIDefinition{}
	spec := APISpec{APIDefinition: &ad}
	base := BaseMiddleware{Spec: &spec, Gw: ts.Gw}
	base.Spec.EnableContextVars = false
=======
	ad := &apidef.APIDefinition{}
	spec := &APISpec{APIDefinition: ad}
	base := &BaseMiddleware{Spec: spec, Gw: ts.Gw}

>>>>>>> 155e11bb3... [TT-13819] Benchmark updates, session limiter workaround for test goroutine leak (#6826)
	transform := TransformMiddleware{base}

	if err := transformBody(r, tmeta, &transform); err != nil {
		t.Fatalf("wanted nil error, got %v", err)
	}
	gotBs, err := ioutil.ReadAll(r.Body)
	if err != nil {
		t.Fatal(err)
	}
	if got := string(gotBs); got != want {
		t.Fatalf("wanted body %q, got %q", want, got)
	}
}

func testPrepareTransformJSONMarshalArray(tb testing.TB) (tmeta *TransformSpec, in string) {
	tmeta = &TransformSpec{}
	tmpl := `[{{ range $key, $value := .array }}{{ if $key }},{{ end }}{{ .abc }}{{ end }}]`
	tmeta.TemplateData.Input = apidef.RequestXML
	tmeta.Template = textTemplate.Must(apidef.Template.New("").Parse(tmpl))

	tmeta.TemplateData.Input = apidef.RequestJSON
	in = `[{"abc": 123}, {"abc": 456}]`

	return tmeta, in
}

func TestTransformJSONMarshalJSONArrayInput(t *testing.T) {
	tmeta, in := testPrepareTransformJSONMarshalArray(t)

	want := `[123,456]`
	r := TestReq(t, "GET", "/", in)

	ts := StartTest(nil)
	defer ts.Close()

<<<<<<< HEAD
	ad := apidef.APIDefinition{}
	spec := APISpec{APIDefinition: &ad}
	base := BaseMiddleware{Spec: &spec, Gw: ts.Gw}
	base.Spec.EnableContextVars = false
=======
	ad := &apidef.APIDefinition{}
	spec := &APISpec{APIDefinition: ad}
	base := &BaseMiddleware{Spec: spec, Gw: ts.Gw}

>>>>>>> 155e11bb3... [TT-13819] Benchmark updates, session limiter workaround for test goroutine leak (#6826)
	transform := TransformMiddleware{base}

	if err := transformBody(r, tmeta, &transform); err != nil {
		t.Fatalf("wanted nil error, got %v", err)
	}
	gotBs, err := ioutil.ReadAll(r.Body)
	if err != nil {
		t.Fatal(err)
	}
	if got := string(gotBs); got != want {
		t.Fatalf("wanted body %q, got %q", want, got)
	}
}

func BenchmarkTransformJSONMarshal(b *testing.B) {
	b.ReportAllocs()

	tmeta, in := testPrepareTransformJSONMarshal("xml")
	ts := StartTest(nil)
	defer ts.Close()

<<<<<<< HEAD
	spec := APISpec{}
	base := BaseMiddleware{Spec: &spec, Gw: ts.Gw}
	base.Spec.EnableContextVars = false
=======
	ad := &apidef.APIDefinition{}
	spec := &APISpec{APIDefinition: ad}
	base := &BaseMiddleware{Spec: spec, Gw: ts.Gw}

>>>>>>> 155e11bb3... [TT-13819] Benchmark updates, session limiter workaround for test goroutine leak (#6826)
	transform := TransformMiddleware{base}

	for i := 0; i < b.N; i++ {
		r := TestReq(b, "GET", "/", in)
		if err := transformBody(r, tmeta, &transform); err != nil {
			b.Fatalf("wanted nil error, got %v", err)
		}
	}
}

func TestTransformXMLMarshal(t *testing.T) {
	assert := func(t *testing.T, input string, tmpl string, output string, inputType apidef.RequestInputType) {
		tmeta := testPrepareTransformXMLMarshal(tmpl, inputType)
		r := TestReq(t, "GET", "/", input)

		ts := StartTest(nil)
		defer ts.Close()

<<<<<<< HEAD
		ad := apidef.APIDefinition{}
		spec := APISpec{APIDefinition: &ad}
		base := BaseMiddleware{Spec: &spec, Gw: ts.Gw}
		base.Spec.EnableContextVars = false
=======
		ad := &apidef.APIDefinition{}
		spec := &APISpec{APIDefinition: ad}
		base := &BaseMiddleware{Spec: spec, Gw: ts.Gw}

>>>>>>> 155e11bb3... [TT-13819] Benchmark updates, session limiter workaround for test goroutine leak (#6826)
Merge Conflict Artifacts

The file contains unresolved merge conflict artifacts (e.g., <<<<<<< HEAD, =======, >>>>>>>). These should be resolved before merging.

<<<<<<< HEAD
func (l *SessionLimiter) doRollingWindowWrite(key, rateLimiterKey, rateLimiterSentinelKey string,
	currentSession *user.SessionState,
	store storage.Handler,
	globalConf *config.Config,
	apiLimit *user.APILimit, dryRun bool) bool {
=======
// Encourage reuse in NewSessionLimiter.
var sessionLimiterBucketStore = memorycache.New()

// NewSessionLimiter initializes the session limiter.
//
// The session limiter initializes the storage required for rate limiters.
// It supports two storage types: `redis` and `local`. If redis storage is
// configured, then redis will be used. If local storage is configured, then
// in-memory counters will be used. If no storage is configured, it falls
// back onto the default gateway storage configuration.
func NewSessionLimiter(ctx context.Context, conf *config.Config, drlManager *drl.DRL) SessionLimiter {
	sessionLimiter := SessionLimiter{
		ctx:         ctx,
		drlManager:  drlManager,
		config:      conf,
		bucketStore: sessionLimiterBucketStore,
	}

	log.Infof("[RATELIMIT] %s", conf.RateLimit.String())
>>>>>>> 155e11bb3... [TT-13819] Benchmark updates, session limiter workaround for test goroutine leak (#6826)
Merge Conflict Artifacts

The file contains unresolved merge conflict artifacts (e.g., <<<<<<< HEAD, =======, >>>>>>>). These should be resolved before merging.

<<<<<<< HEAD
.github/workflows/sync-automation.yml   @TykTechnologies/devops
.github/workflows/pac.yml               @TykTechnologies/devops
/repo-policy/                           @TykTechnologies/devops
=======

# Core API Squad ownership of CI test pipeline and developer tooling.

/bin/                                   @TykTechnologies/core-api-squad
.github/workflows/ci-tests.yml          @TykTechnologies/core-api-squad
.github/workflows/release-tests.yml     @TykTechnologies/core-api-squad
/.taskfiles/                            @TykTechnologies/core-api-squad
/Dockerfile                             @TykTechnologies/core-api-squad
/docker/                                @TykTechnologies/core-api-squad
/Makefile                               @TykTechnologies/core-api-squad
/Taskfile.yml                           @TykTechnologies/core-api-squad
/docker-compose.yml                     @TykTechnologies/core-api-squad
>>>>>>> 155e11bb3... [TT-13819] Benchmark updates, session limiter workaround for test goroutine leak (#6826)

Copy link
Contributor

github-actions bot commented Jan 16, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Remove merge conflict markers to ensure the code is clean and functional

Resolve the merge conflict markers (<<<<<<<, =======, >>>>>>>) in the test files to ensure
the code compiles and runs correctly.

gateway/mw_transform_test.go [40-51]

-+<<<<<<< HEAD
-ad := apidef.APIDefinition{}
-spec := APISpec{APIDefinition: &ad}
-base := BaseMiddleware{Spec: &spec, Gw: ts.Gw}
-base.Spec.EnableContextVars = false
-+=======
 ad := &apidef.APIDefinition{}
 spec := &APISpec{APIDefinition: ad}
 base := &BaseMiddleware{Spec: spec, Gw: ts.Gw}
-+
-+>>>>>>> 155e11bb3... [TT-13819] Benchmark updates, session limiter workaround for test goroutine leak (#6826)
Suggestion importance[1-10]: 10

Why: The merge conflict markers must be resolved for the code to compile and function correctly. This is a critical issue that directly impacts the integrity of the codebase.

10
General
Add a check to ensure the coverage directory exists before running benchmarks

Ensure the coverage directory exists before running the benchmarks to prevent
potential file creation errors.

bin/ci-benchmark.sh [4-16]

-+	./gateway.test -test.run=^$ -test.bench=$benchRegex -test.count=1 \
-+			-test.benchtime 30s -test.benchmem \
-+			-test.cpuprofile=coverage/$benchmark-cpu.out \
-+			-test.memprofile=coverage/$benchmark-mem.out \
-+			-test.trace=coverage/$benchmark-trace.out
+mkdir -p coverage
+./gateway.test -test.run=^$ -test.bench=$benchRegex -test.count=1 \
+			-test.benchtime 30s -test.benchmem \
+			-test.cpuprofile=coverage/$benchmark-cpu.out \
+			-test.memprofile=coverage/$benchmark-mem.out \
+			-test.trace=coverage/$benchmark-trace.out
Suggestion importance[1-10]: 8

Why: Ensuring the coverage directory exists prevents potential runtime errors during benchmark execution, improving the robustness of the script.

8
Ensure thread safety and proper error handling in the assertTokensLen helper function

Ensure the assertTokensLen helper function is thread-safe and does not introduce
race conditions when accessing shared resources.

gateway/oauth_manager_test.go [1315-1321]

-+func assertTokensLen(t *testing.T, storageManager storage.Handler, storageKey string, expectedTokensLen int) {
-+	t.Helper()
-+	nowTs := time.Now().Unix()
-+	startScore := strconv.FormatInt(nowTs, 10)
-+	tokens, _, err := storageManager.GetSortedSetRange(storageKey, startScore, "+inf")
-+	assert.NoError(t, err)
-+	assert.Equal(t, expectedTokensLen, len(tokens))
-+}
+func assertTokensLen(t *testing.T, storageManager storage.Handler, storageKey string, expectedTokensLen int) {
+	t.Helper()
+	nowTs := time.Now().Unix()
+	startScore := strconv.FormatInt(nowTs, 10)
+	tokens, _, err := storageManager.GetSortedSetRange(storageKey, startScore, "+inf")
+	if err != nil {
+		t.Fatalf("Error accessing storage: %v", err)
+	}
+	if len(tokens) != expectedTokensLen {
+		t.Fatalf("Expected %d tokens, got %d", expectedTokensLen, len(tokens))
+	}
+}
Suggestion importance[1-10]: 7

Why: The improved code introduces better error handling, which enhances the reliability of the test function. However, the suggestion does not explicitly address thread safety, which limits its impact.

7
Confirm that disabling rate limits and quotas in tests does not impact other tests or production settings

Verify that disabling rate limits and quotas in tests (spec.DisableRateLimit and
spec.DisableQuota) does not unintentionally affect other test cases or production
configurations.

gateway/api_definition_test.go [1020-1021]

-+		spec.DisableRateLimit = true
-+		spec.DisableQuota = true
+spec.DisableRateLimit = true
+spec.DisableQuota = true
+// Ensure these settings are scoped only to this test and reset after execution.
Suggestion importance[1-10]: 6

Why: While the suggestion to verify the scope of spec.DisableRateLimit and spec.DisableQuota is valid, it is not actionable in its current form. Adding a comment to ensure these settings are scoped to the test is a minor improvement.

6

@titpetric titpetric marked this pull request as ready for review January 16, 2025 00:31
@TykTechnologies TykTechnologies deleted a comment from github-actions bot Jan 16, 2025
Copy link
Contributor

💥 CI tests failed 🙈

git-state

diff --git a/certs/mock/mock.go b/certs/mock/mock.go
index f55e3a8..1b6f712 100644
--- a/certs/mock/mock.go
+++ b/certs/mock/mock.go
@@ -13,9 +13,8 @@ import (
 	x509 "crypto/x509"
 	reflect "reflect"
 
-	gomock "go.uber.org/mock/gomock"
-
 	certs "github.com/TykTechnologies/tyk/certs"
+	gomock "go.uber.org/mock/gomock"
 )
 
 // MockCertificateManager is a mock of CertificateManager interface.

Please look at the run or in the Checks tab.

@titpetric titpetric enabled auto-merge (squash) January 16, 2025 00:52
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
57.9% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@titpetric titpetric merged commit 9dadb6f into release-5.2 Jan 16, 2025
12 of 20 checks passed
@titpetric titpetric deleted the merge/release-5.2/155e11bb38c082d4bd880472531c1a1ad2d66218 branch January 16, 2025 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants