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.

Sign up for GitHub

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

Jump to bottom

[TT-12494] Fix flaky TestCacheEtag and related cache tests #6508

Merged
merged 9 commits into from
Sep 12, 2024
Merged

[TT-12494] Fix flaky TestCacheEtag and related cache tests #6508

merged 9 commits into from
Sep 12, 2024

Conversation

titpetric
Copy link
Contributor

@titpetric titpetric commented Sep 12, 2024 •
edited
Loading

User description

This considers some necessary improvements:

  • revert test to run e2e-combined (e2e only has some CI specific issues when running, not seen on local)
  • add cancellation group to CI tests, new commits on PR cancel old CI Tests run
  • pass only the merged .cov file to sonarcloud (not all of them)
  • https://tyktech.atlassian.net/browse/TT-12494 (tests issue, deleting caches, defer)
  • fix some dialing out in tests/regression (httpbin/google)

gateway_test.go: The tests are flaky on account they delete the redis cache. Deleting a redis cache from a test interferes with other tests that also read/write cache data from redis.

tests/regression: using external resources (confirmed fix)

PR Type

enhancement, tests

Description

  • Added concurrency settings to the CI workflow to ensure only one runner per PR and commit, canceling old runs on new commits.
  • Updated the test command to test:e2e-combined to address CI-specific issues.
  • Changed the sonar coverage report path to use a merged .cov file (gateway-all.cov).
  • Simplified the package listing command in the test task configuration.

Changes walkthrough 📝

Relevant files
Enhancement
ci-tests.yml
Enhance CI workflow with concurrency and test improvements

.github/workflows/ci-tests.yml

  • Added concurrency settings to CI tests to cancel old runs on new
    commits.
  • Changed test command to test:e2e-combined.
  • Updated sonar coverage report path to gateway-all.cov.
    		•  +11/-2   
    test.yml
    Simplify package listing command in test task                       

    .taskfiles/test.yml

    • Simplified the package listing command in the test task.
    		 +1/-1     

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

    @titpetric titpetric requested a review from a team as a code owner September 12, 2024 11:57
    @github-actions GitHub Actions
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No key issues to review

    @github-actions GitHub Actions
    Copy link
    Contributor

    github-actions bot commented Sep 12, 2024 •
    edited
    Loading

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Check and ensure the task command is installed before using it

    Ensure that the task command is installed and available in the CI environment before
    calling task --version, to prevent potential failures during the execution of the
    workflow.

    .github/workflows/ci-tests.yml [97]

    -task --version
+- name: Ensure task is installed
+  run: |
+    if ! command -v task &> /dev/null
+    then
+      echo "task could not be found, installing..."
+      # Add installation command here
+    fi
+- name: Check task version
+  run: task --version
    Suggestion importance[1-10]: 8

    Why: Ensuring the task command is installed before using it is crucial to prevent workflow failures, making this a significant improvement for reliability.

    		8
    Enhancement
    Use a configurable environment variable for the test timeout setting

    Replace the hardcoded timeout value with a configurable environment variable or a
    workflow input to make the timeout setting more flexible and maintainable.

    .github/workflows/ci-tests.yml [129]

    -task test:e2e-combined args="-race -timeout=15m"
+task test:e2e-combined args="-race -timeout=${{ env.TEST_TIMEOUT }}"
    Suggestion importance[1-10]: 7

    Why: Replacing the hardcoded timeout with a configurable variable enhances flexibility and maintainability, though it is not critical for functionality.

    		7
    Best practice
    Use a more precise pattern for coverage report paths to ensure accuracy

    Specify a more precise coverage report path pattern to ensure that only relevant
    coverage files are included, avoiding potential inclusion of unwanted coverage data.

    .github/workflows/ci-tests.yml [156]

    --Dsonar.go.coverage.reportPaths=coverage/gateway-all.cov
+-Dsonar.go.coverage.reportPaths=coverage/gateway-*.cov
    Suggestion importance[1-10]: 6

    Why: Specifying a more precise pattern for coverage report paths can improve accuracy, but the impact is relatively minor unless there are multiple coverage files that could be incorrectly included.

    		6
    Narrow the pattern matching for release branches to prevent unintended CI triggers

    Consider using a more specific pattern for the release branches in the CI triggers
    to avoid unintentional builds from non-release branches that might use similar
    naming conventions.

    .github/workflows/ci-tests.yml [18]

     branches:
   - master
-  - release-**
+  - release-*
    Suggestion importance[1-10]: 5

    Why: The suggestion to narrow the pattern for release branches can help avoid unintended CI triggers, but the improvement is minor since it may not significantly impact the workflow unless there are similarly named branches.

    		5

    @github-actions GitHub Actions
    Copy link
    Contributor

    github-actions bot commented Sep 12, 2024 •
    edited
    Loading

    API Changes

    no api changes detected

    @titpetric titpetric changed the title Testing/ci improvements [TT-12494] Fix flaky TestCacheEtag and related cache tests Sep 12, 2024
    @titpetric titpetric enabled auto-merge (squash) September 12, 2024 14:09
    @titpetric titpetric mentioned this pull request Sep 12, 2024
    Merged
    @sonarcloud SonarCloud
    Copy link

    sonarcloud bot commented Sep 12, 2024

    Quality Gate Passed Quality Gate passed

    Issues
    0 New issues
    0 Accepted issues

    Measures
    0 Security Hotspots
    0.0% Coverage on New Code
    0.0% Duplication on New Code

    See analysis details on SonarCloud

    @titpetric titpetric merged commit bc067e7 into master Sep 12, 2024
    27 checks passed
    @titpetric titpetric deleted the testing/ci-improvements branch September 12, 2024 17:27
    titpetric added a commit that referenced this pull request Sep 12, 2024
    @titpetric
    [TT-12494] Fix flaky TestCacheEtag and related cache tests (#6508)
    dea3728 
    This considers some necessary improvements:

- [x] revert test to run e2e-combined (e2e only has some CI specific
issues when running, not seen on local)
- [x] add cancellation group to CI tests, new commits on PR cancel old
CI Tests run
- [x] pass only the merged .cov file to sonarcloud (not all of them)
- [x] https://tyktech.atlassian.net/browse/TT-12494 (tests issue,
deleting caches, defer)

The tests are flaky on account they delete the redis cache. Deleting a
redis cache from a test interferes with other tests that also read/write
cache data from redis.

___

enhancement, tests

___

- Added concurrency settings to the CI workflow to ensure only one
runner per PR and commit, canceling old runs on new commits.
- Updated the test command to `test:e2e-combined` to address CI-specific
issues.
- Changed the sonar coverage report path to use a merged `.cov` file
(`gateway-all.cov`).
- Simplified the package listing command in the test task configuration.

___

<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>ci-tests.yml</strong><dd><code>Enhance CI workflow with
concurrency and test improvements</code></dd></summary>
<hr>

.github/workflows/ci-tests.yml

<li>Added concurrency settings to CI tests to cancel old runs on new
<br>commits.<br> <li> Changed test command to
<code>test:e2e-combined</code>.<br> <li> Updated sonar coverage report
path to <code>gateway-all.cov</code>.<br>

</details>

  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6508/files#diff-03609cb60b0c6e92fb771eb8787d6722b8c31ca4c03eabc788e147acd8c6fb43">+11/-2</a>&nbsp;
&nbsp; </td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>test.yml</strong><dd><code>Simplify package listing
command in test task</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary>
<hr>

.taskfiles/test.yml

- Simplified the package listing command in the test task.

</details>

  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6508/files#diff-f1fbe7f7f14888019b8845634ed008e1c43f6e5a5c0b2707336fc7f8e15a36fb">+1/-1</a>&nbsp;
&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 <tit@tyk.io>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @jeffy-mathew jeffy-mathew jeffy-mathew approved these changes

    Assignees
    No one assigned
    Labels
    enhancement 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 @jeffy-mathew