Skip to content

[TT-12186] Fixes TestOAS_ExtractTo_ResetAPIDefinition with a valid event config#6321

Merged
titpetric merged 1 commit into
masterfrom
fix/tt-12186/fix-webhook-migration-test
Jun 3, 2024
Merged

[TT-12186] Fixes TestOAS_ExtractTo_ResetAPIDefinition with a valid event config#6321
titpetric merged 1 commit into
masterfrom
fix/tt-12186/fix-webhook-migration-test

Conversation

@titpetric

@titpetric titpetric commented Jun 3, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

User description

The migration wasn't handled correctly in test, test only change

PR Type

Bug fix, Tests

Description

  • Added a valid event handler configuration to the TestOAS_ExtractTo_ResetAPIDefinition test to ensure proper migration handling.
  • Removed an incorrect assertion related to event handlers in the test.
  • Updated import statements for better organization and readability.

Changes walkthrough 📝

Relevant files
Tests
oas_test.go
Fix and enhance `TestOAS_ExtractTo_ResetAPIDefinition` test.

apidef/oas/oas_test.go

  • Added valid event handler configuration to
    TestOAS_ExtractTo_ResetAPIDefinition.
  • Removed an incorrect assertion in the test.
  • Updated import statements for better organization.
    		•  +14/-4   

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

    @github-actions

    github-actions Bot commented Jun 3, 2024

    Copy link
    Copy Markdown
    Contributor

    PR Description updated to latest commit (a35065f)

    @github-actions

    github-actions Bot commented Jun 3, 2024

    Copy link
    Copy Markdown
    Contributor

    API Changes

    no api changes detected

    @github-actions

    github-actions Bot commented Jun 3, 2024

    Copy link
    Copy Markdown
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the PR is focused on a specific test case with a clear scope of changes, involving a manageable amount of code modifications and updates.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    No

    🔒 Security concerns

    No
    Code feedback:
    relevant fileapidef/oas/oas_test.go
    suggestion      

    Consider validating the HandlerMeta map for necessary keys and values to prevent runtime errors. This can be done by adding a check after the map is populated to ensure all required fields are present and correctly formatted. [important]
    relevant line"target_path": "https://webhook.site/uuid",

    @github-actions

    github-actions Bot commented Jun 3, 2024

    Copy link
    Copy Markdown
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use environment variables for external dependencies in tests

    To ensure that the test remains effective and does not depend on external services,
    consider using a mock server or environment variables for the webhook URL.

    apidef/oas/oas_test.go [141]

    -"target_path": "https://webhook.site/uuid",
+"target_path": os.Getenv("WEBHOOK_TEST_URL"),
    Suggestion importance[1-10]: 9

    Why: Using environment variables for external dependencies in tests is a best practice that ensures tests are not dependent on external services, making them more reliable and easier to run in different environments.

    		9
    Possible issue
    Add error handling after setting event handlers

    It's recommended to check for errors after setting up the event handlers to ensure that
    the map and its elements are correctly initialized without any runtime panics.

    apidef/oas/oas_test.go [136-143]

    -a.EventHandlers.Events = map[apidef.TykEvent][]apidef.EventHandlerTriggerConfig{
+err := a.SetEventHandlers(map[apidef.TykEvent][]apidef.EventHandlerTriggerConfig{
     event.QuotaExceeded: {
         {
             Handler: event.WebHookHandler,
             HandlerMeta: map[string]interface{}{
-                "target_path": "https://webhook.site/uuid",
+                "target_path": webhookURL,
             },
         },
     },
+})
+if err != nil {
+    t.Fatal(err)
 }
    Suggestion importance[1-10]: 8

    Why: Adding error handling after setting event handlers is important to catch potential issues early and prevent runtime panics, which enhances the robustness of the test.

    		8
    Maintainability
    Replace hardcoded webhook URL with a constant

    Consider using a constant for the webhook URL to avoid hardcoding it directly in the test
    function. This makes the code easier to maintain and update.

    apidef/oas/oas_test.go [141]

    -"target_path": "https://webhook.site/uuid",
+"target_path": webhookURL,
    Suggestion importance[1-10]: 7

    Why: Using a constant for the webhook URL improves maintainability by making it easier to update the URL in the future. However, this is a minor improvement and not crucial for the functionality of the test.

    		7
    Verify the necessity of the config import to maintain clean code

    Since the import of 'github.com/TykTechnologies/tyk/config' was removed and added back,
    ensure it is actually used in the test file to avoid unnecessary imports which can lead to
    confusion and slight performance issues.

    apidef/oas/oas_test.go [13]

    -"github.com/TykTechnologies/tyk/config"
+// Ensure usage of config or remove the import if not needed
    Suggestion importance[1-10]: 6

    Why: Verifying the necessity of the import helps maintain clean code and avoid confusion, but it is a minor issue and does not significantly impact the functionality or performance of the code.

    		6

    @sonarqubecloud

    sonarqubecloud Bot commented Jun 3, 2024

    Copy link
    Copy Markdown

    Quality Gate Passed 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

    @titpetric
    titpetric enabled auto-merge (squash) June 3, 2024 13:06
    @titpetric
    titpetric merged commit 0d1183c into master Jun 3, 2024
    @titpetric
    titpetric deleted the fix/tt-12186/fix-webhook-migration-test branch June 3, 2024 13:52
    nerdydread pushed a commit that referenced this pull request Sep 6, 2024
    @nerdydread
    [TT-12186] Fixes TestOAS_ExtractTo_ResetAPIDefinition with a valid ev…
    67412f0 
    …ent config (#6321)

### **User description**
The migration wasn't handled correctly in test, test only change


___

### **PR Type**
Bug fix, Tests


___

### **Description**
- Added a valid event handler configuration to the
`TestOAS_ExtractTo_ResetAPIDefinition` test to ensure proper migration
handling.
- Removed an incorrect assertion related to event handlers in the test.
- Updated import statements for better organization and readability.


___



### **Changes walkthrough** 📝
<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Tests
</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>oas_test.go</strong><dd><code>Fix and enhance
`TestOAS_ExtractTo_ResetAPIDefinition` test.</code></dd></summary>
<hr>

apidef/oas/oas_test.go
<li>Added valid event handler configuration to
<br><code>TestOAS_ExtractTo_ResetAPIDefinition</code>.<br> <li> Removed
an incorrect assertion in the test.<br> <li> Updated import statements
for better organization.<br>


</details>
    

  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6321/files#diff-74029ee88132d30d6478c96a35f8bb2200e0c8e6f42f2c9b147dc6bb7ce74644">+14/-4</a>&nbsp;
&nbsp; </td>
</tr>                    
</table></td></tr></tr></tbody></table>

___

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

Co-authored-by: Tit Petric <[email protected]>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @andrei-tyk andrei-tyk andrei-tyk approved these changes

    Assignees

    No one assigned

    Labels

    Bug fix Review effort [1-5]: 2 Tests

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @titpetric @andrei-tyk