[TT-13175] Refactor apply policies test

jeffy-mathew commented 2 months ago

User description

Description

refactor apply policies tests to internal/policy

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[x] Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

[ ] I ensured that the documentation is up to date
[ ] I explained why this PR updates go.mod in detail with reasoning why it's required
[ ] I would like a code coverage CI quality gate exception and have explained why

PR Type

Tests

Description

Refactored apply policies test cases by moving them from gateway/policy_test.go to internal/policy/apply_test.go.
Removed old test functions TestApplyPolicies and BenchmarkApplyPolicies from gateway/policy_test.go.
Added new test functions TestService_Apply and BenchmarkService_Apply in internal/policy/apply_test.go.
Introduced new policy entries in internal/policy/testdata/policies.json for enhanced test coverage.

Changes walkthrough 📝

Relevant files

Tests

policy_test.go `Remove apply policies test cases from gateway` gateway/policy_test.go Removed extensive test cases related to policy application. Deleted the `TestApplyPolicies` and `BenchmarkApplyPolicies` functions. Removed imports related to deleted test cases.	+0/-1088
apply_test.go `Add apply policies test cases to internal/policy` internal/policy/apply_test.go Added new test cases for policy application. Introduced `TestService_Apply` and `BenchmarkService_Apply` functions. Added helper function `testPrepareApplyPolicies`. Imported necessary packages for new tests.	+1077/-0
policies.json `Add new policy entries for testing` internal/policy/testdata/policies.json Added new policy entries for testing. Introduced `acl_with_allowed_url` and `rate_limit` policies.	+37/-0

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

github-actions[bot] commented 2 months ago

API Changes

no api changes detected

github-actions[bot] commented 2 months ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review

Code Duplication
The test cases and structures in 'internal/policy/apply_test.go' are largely duplicated from the previous test suite in 'gateway/policy_test.go'. Consider refactoring to share common test utilities and data structures to reduce duplication and maintenance overhead. Missing Error Handling
In the new test functions `TestService_Apply` and `BenchmarkService_Apply`, there is no handling or assertion for potential errors returned from the `service.Apply(sess)` method. This could lead to missed test failures during benchmarking and unit testing.

github-actions[bot] commented 2 months ago

PR Code Suggestions ✨

Category	Suggestion	Score
Best practice	Use `require.NoError` to immediately halt test execution on setup errors ___ Instead of using `assert.NoError` for setup steps in tests, use `require.NoError` to immediately fail the test upon error, preventing any subsequent steps from running. [internal/policy/apply_test.go [181]](https://github.com/TykTechnologies/tyk/pull/6585/files#diff-5af7e299a6b0ce11e22f8aa4a01854b1151f4b54dccc68f0cd1cbedee5aed7c8R181-R181) ```diff -assert.NoError(tb, err) +require.NoError(tb, err) ``` Suggestion importance[1-10]: 9 Why: Using `require.NoError` is a best practice in tests to prevent further execution when a critical setup step fails, ensuring that tests do not proceed with invalid states.	9
Best practice	Improve error handling in tests to ensure accurate reporting of failures ___ Consider handling the error from `service.Apply(sess)` directly in the test case to ensure that test failures are reported accurately. [internal/policy/apply_test.go [1210-1212]](https://github.com/TykTechnologies/tyk/pull/6585/files#diff-5af7e299a6b0ce11e22f8aa4a01854b1151f4b54dccc68f0cd1cbedee5aed7c8R1210-R1212) ```diff if err := service.Apply(sess); err != nil { - assert.ErrorContains(t, err, tc.errMatch) + if !assert.ErrorContains(t, err, tc.errMatch) { + t.Errorf("Unexpected error: %v", err) + } return } ``` Suggestion importance[1-10]: 8 Why: The suggestion enhances error handling by ensuring that unexpected errors are reported, which improves test reliability and debugging.	8
Performance	Optimize the reversal of slices by using a manual loop instead of importing the `slices` package ___ Replace the use of `slices.Reverse` with a more efficient manual reverse loop to avoid importing an additional package. [internal/policy/apply_test.go [1194]](https://github.com/TykTechnologies/tyk/pull/6585/files#diff-5af7e299a6b0ce11e22f8aa4a01854b1151f4b54dccc68f0cd1cbedee5aed7c8R1194-R1194) ```diff -slices.Reverse(copyPols) +for i, j := 0, len(copyPols)-1; i < j; i, j = i+1, j-1 { + copyPols[i], copyPols[j] = copyPols[j], copyPols[i] +} ``` Suggestion importance[1-10]: 7 Why: The suggestion to replace `slices.Reverse` with a manual loop is valid and can improve performance by avoiding an additional package import. However, the performance gain might be minor in this context.	7
Maintainability	Improve code readability by using more descriptive variable names ___ Use a more descriptive variable name than `tb` for the testing object to enhance code readability. [internal/policy/apply_test.go [177]](https://github.com/TykTechnologies/tyk/pull/6585/files#diff-5af7e299a6b0ce11e22f8aa4a01854b1151f4b54dccc68f0cd1cbedee5aed7c8R177-R177) ```diff -func testPrepareApplyPolicies(tb testing.TB) (policy.Service, []testApplyPoliciesData) { +func testPrepareApplyPolicies(t testing.TB) (policy.Service, []testApplyPoliciesData) { ``` Suggestion importance[1-10]: 5 Why: While changing `tb` to `t` slightly improves readability, the improvement is minimal as both are common shorthand in Go testing.	5