[TT-7639] Add regression test for JSVM failure

[titpetric]

User description

https://tyktech.atlassian.net/browse/TT-7639

Note: not for merge (yet), waiting on additional details.

---

Type

Tests, Enhancement

---

Description

• Added a regression test to verify correct handling of context variables within the JSVM in Tyk Gateway.
• The test checks the behavior when context variables are enabled and disabled, ensuring the system's robustness.
• Included a new API definition JSON file specifically designed for this test scenario.

---

Changes walkthrough

	Relevant files
Tests	issue_7639_test.go Add Regression Test for JSVM Context Variables Handling    tests/regression/issue_7639_test.go Added a new regression test Test_Issue7639 to verify the behavior of context variables being enabled or disabled. The test starts a test server, loads an API definition, and runs two test cases (GET and POST) with context variables both enabled and disabled. +48/-0
Enhancement	issue-7639-apidef.json Add Test API Definition for Regression Test                            tests/regression/testdata/issue-7639-apidef.json Introduced a new API definition JSON file for testing purposes. The API definition includes configurations for a mock bug-report API with various settings including custom middleware for post-processing. +455/-0

---

✨ PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[github-actions]

PR Description updated to latest commit (2a14081)

[github-actions]

API Changes

no api changes detected

[github-actions]

PR Review

⏱️ Estimated effort to review [1-5]	2, because the PR primarily involves adding a new test case and a new API definition JSON file. The test case is straightforward, focusing on testing the behavior of context variables in the Tyk Gateway. The JSON file, while large, is a standard API definition for Tyk and doesn't require deep inspection of logic but rather a validation of its structure and relevance to the test case.
🧪 Relevant tests	Yes
🔍 Possible issues	Hardcoded Status Code: The test cases assert for a status code of 500, which might not be the best choice for a successful test scenario. Typically, a status code of 200 or another 2xx status would indicate success, while a 500 status code suggests a server error. This could be misleading when interpreting the test results.
🔒 Security concerns	No

Code feedback:

---

relevant file	tests/regression/issue_7639_test.go
suggestion	Consider using more appropriate status codes for successful test scenarios instead of 500. Status codes in the 2xx range, such as 200, are typically used to indicate success. This change would make the test results more intuitive and aligned with HTTP standards. [important]
relevant line	Code: 500,

---

---

✨ Review tool usage guide:

---

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

• When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

• With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

[github-actions]

PR Code Suggestions

Category	Suggestions
Enhancement	Convert tests to use table-driven approach for better maintainability. Consider using table-driven tests to simplify your test cases and make them more maintainable. This approach allows you to define test cases in a slice of structs and range over them, reducing code duplication and improving readability. tests/regression/issue_7639_test.go [25-46] -for _, enable := range []bool{true, false} { - spec.EnableContextVars = enable +testCases := []struct { + enableContextVars bool + expectedCode int + method string + data string +}{ + {true, 500, "GET", ""}, + {true, 500, "POST", string(form.Encode())}, + {false, 500, "GET", ""}, + {false, 500, "POST", string(form.Encode())}, +} +for _, tc := range testCases { + spec.EnableContextVars = tc.enableContextVars ts.Gw.LoadAPI(spec) - t.Run(fmt.Sprintf("Context vars enabled=%v", enable), func(t *testing.T) { - ts.Run(t, []test.TestCase{ - { - Method: "GET", - Path: "/bug-report/", - Code: 500, - }, - { - Method: "POST", - Path: "/bug-report/", - Headers: map[string]string{"Content-Type": "application/x-www-form-urlencoded"}, - Data: string(form.Encode()), - Code: 500, - }, - }...) + t.Run(fmt.Sprintf("Context vars enabled=%v, Method=%s", tc.enableContextVars, tc.method), func(t *testing.T) { + ts.Run(t, test.TestCase{ + Method: tc.method, + Path: "/bug-report/", + Headers: map[string]string{"Content-Type": "application/x-www-form-urlencoded"}, + Data: tc.data, + Code: tc.expectedCode, + }) }) }
Best practice	Add error handling for loading API specifications. It's a good practice to check for errors when loading API specifications and handling them appropriately. This can prevent the test from proceeding with invalid or unexpected states, leading to clearer, more reliable tests. tests/regression/issue_7639_test.go [17] -spec := loadAPISpec(t, "testdata/issue-7639-apidef.json") +spec, err := loadAPISpec(t, "testdata/issue-7639-apidef.json") +if err != nil { + t.Fatalf("Failed to load API spec: %v", err) +}
	Use constants for HTTP status codes instead of hardcoding. Instead of hardcoding the expected response code as 500, consider using constants from the net/http package for better readability and maintainability. This makes the code more understandable at a glance and easier to update in the future. tests/regression/issue_7639_test.go [36-43] -Code: 500, +Code: http.StatusInternalServerError,
	Reset spec object state after each test case to ensure test isolation. To ensure the test environment is clean and does not affect other tests, consider resetting the spec object's state after each test case. This can be done by re-loading the API spec or by implementing a reset function. tests/regression/issue_7639_test.go [26-27] +originalSpec := spec +defer func() { spec = originalSpec }() spec.EnableContextVars = enable ts.Gw.LoadAPI(spec)
Maintainability	Define form data keys as constants for clarity and maintainability. For better test clarity and to avoid magic numbers, consider defining the form data keys ("foo", "bar", "baz") as constants at the beginning of your test file. This makes it easier to update and understand the purpose of these keys. tests/regression/issue_7639_test.go [21-23] -form.Add("foo", "swiggetty") -form.Add("bar", "swoggetty") -form.Add("baz", "swoogetty") +const ( + keyFoo = "foo" + keyBar = "bar" + keyBaz = "baz" +) +form.Add(keyFoo, "swiggetty") +form.Add(keyBar, "swoggetty") +form.Add(keyBaz, "swoogetty")

---

✨ Improve tool usage guide:

---

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

• When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

• With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud