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

[TT-10532] Update taskfile, CI tests #6350

Merged
merged 18 commits into from
Jun 21, 2024

Conversation

titpetric
Copy link
Contributor

@titpetric titpetric commented Jun 17, 2024

This PR implements:

Taskfile.yml, .taskfiles/test.yml, taskfiles/deps/Taskfile.yml:

  • added test:e2e and test:e2e-combined. By default they run without -race.
  • included new CI dependencies in deps taskfile (provided with tykio/ci-tools for github CI, this is for local development)

ci-tests.yml:

  • CI runs task test:e2e-combined args="-race -timeout=15m"
  • CI runs task test:coverage to print per-package coverage report
  • CI runs task test:report at to print slowest tests (only available in e2e-combined)
  • CI Pipeline changes: run test environment from taskfiles
    • Cache moved up to happen before make lint (restore cache first)
    • Using tykio/ci-tools to get task, golangci-lint and other CI tooling, silence golangcilint config
    • Removed github comment notification, it's noise - it's a required pipeline

Pipeline for sonarcloud is now pinned to redis7, there is some unnecessary duplication due to the redis matrix and some actions like golangci-lint unfortunately run twice. Our next step should be to make the redis 7 pipeline required, and then remove the redis 5 pipeline, and then sanitize the state in a future review.

storage: test fix for a data race
gateway: rpc_test.go, agreed marking RPC test as flaky due to sensitivity to -race (ok'd with @sredxny)
docker/services: support REDIS_IMAGE to override for matrix test

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

@titpetric titpetric requested a review from a team as a code owner June 17, 2024 12:10
Copy link
Contributor

API Changes

no api changes detected

Copy link
Contributor

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5] 4
🧪 Relevant tests Yes
🔒 Security concerns No
⚡ Key issues to review CI Configuration:
The PR sets CI: 'true' which might skip flaky tests as mentioned in the PR description. This could potentially mask issues that only appear under certain conditions.
Test Coverage:
The use of head -n -2 in the test commands might inadvertently trim important output from the test results, potentially hiding errors or important warnings.
Race Condition Handling:
The PR includes separate tasks for running tests with and without the -race flag. It's crucial to ensure that tests which should be run with the race detector are indeed being executed in that mode, especially since RPC tests are mentioned to be problematic with -race.
Dependency Management:
The PR includes updates to task dependencies and introduces new tasks. It's important to ensure these dependencies are correctly managed to avoid issues in the build pipeline.

Copy link
Contributor

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Best practice
Add error handling to shell commands in variable definitions to enhance robustness

Consider adding error handling for the shell commands used in variable definitions to
prevent failures that could terminate the task unexpectedly. For example, the go mod edit
-json command could fail if not run within a Go module.

.taskfiles/test.yml [39]

-sh: go mod edit -json | jq .Module.Path -r
+sh: go mod edit -json | jq .Module.Path -r || echo "Error: Not a Go module"
 
Suggestion importance[1-10]: 8

Why: Adding error handling to shell commands is a good practice that enhances the robustness of the script. This suggestion addresses potential failures that could terminate the task unexpectedly, making it a valuable improvement.

8
Use a more robust and readable method for string manipulation in the packages variable

Replace the sed command with a more robust method for string manipulation in the packages
variable. Using sed can be error-prone and less readable. Consider using a more direct
method of string replacement that might be provided by the language or framework you are
using.

.taskfiles/test.yml [41]

-sh: go list ./... | sed -e 's|{{.package}}|.|g'
+sh: go list ./... | awk '{gsub("{{.package}}", ".", $0); print $0}'
 
Suggestion importance[1-10]: 6

Why: The suggestion to replace sed with awk for string manipulation is valid as awk can be more readable and less error-prone. However, both commands are commonly used, and the change is more about preference and readability rather than fixing a critical issue.

6
Enhancement
Clarify the descriptions of similar tasks to avoid confusion and provide better context

The integration and integration-combined tasks have very similar descriptions. It might be
beneficial to differentiate these descriptions more clearly to avoid confusion and to
better describe the specific purpose of each task.

.taskfiles/test.yml [31]

-desc: "Run e2e/integration tests"
+desc: "Run individual e2e/integration tests"
 
Suggestion importance[1-10]: 7

Why: Differentiating the descriptions of similar tasks can help avoid confusion and provide better context. This is a useful enhancement for maintainability and clarity, though it is not critical.

7
Improve the default value for the tags variable to enhance clarity and context

Consider using a more specific default value for the tags variable. The current default
value 'goplugin' might not be descriptive enough for all scenarios where this Taskfile is
used. A more specific default could help in understanding the context or purpose of the
tests without needing to specify it every time.

.taskfiles/test.yml [14]

-tags: '{{ .tags | default "goplugin" }}'
+tags: '{{ .tags | default "default-test-tag" }}'
 
Suggestion importance[1-10]: 5

Why: While the suggestion to use a more specific default value for the tags variable could enhance clarity, it is not crucial and does not address any major issues. The current default value 'goplugin' is functional, and changing it to 'default-test-tag' is a minor improvement.

5

@jeffy-mathew jeffy-mathew force-pushed the test/tt-10532/testing-improvements-taskfile-update branch from 8358704 to 9b304fe Compare June 19, 2024 09:14
@titpetric titpetric force-pushed the test/tt-10532/testing-improvements-taskfile-update branch from 3243323 to 777f008 Compare June 20, 2024 12:59
@TykTechnologies TykTechnologies deleted a comment from github-actions bot Jun 20, 2024
@titpetric titpetric changed the title [TT-10531] WIP: Update taskfile [TT-10531] WIP: Update taskfile, CI tests Jun 20, 2024
@TykTechnologies TykTechnologies deleted a comment from github-actions bot Jun 20, 2024
Copy link

sonarcloud bot commented Jun 20, 2024

2 similar comments
Copy link

sonarcloud bot commented Jun 20, 2024

Copy link

sonarcloud bot commented Jun 20, 2024

@titpetric titpetric force-pushed the test/tt-10532/testing-improvements-taskfile-update branch from bbdd7f6 to 8d3fbf3 Compare June 20, 2024 16:43
Copy link

sonarcloud bot commented Jun 20, 2024

@titpetric titpetric changed the title [TT-10531] WIP: Update taskfile, CI tests [TT-10531] Update taskfile, CI tests Jun 20, 2024
@titpetric titpetric changed the title [TT-10531] Update taskfile, CI tests [TT-10532] Update taskfile, CI tests Jun 20, 2024
@titpetric titpetric merged commit 69a084f into master Jun 21, 2024
36 checks passed
@titpetric titpetric deleted the test/tt-10532/testing-improvements-taskfile-update branch June 21, 2024 11:06
titpetric added a commit that referenced this pull request Jun 25, 2024
### **User description**
The change introduced in #6350 changed the exit-code incorrectly. Added
comment.


___

### **PR Type**
bug fix, enhancement


___

### **Description**
- Fixed the `golangci-lint` step in the CI workflow to use
`issues-exit-code=0` to ensure proper data passing to SonarCloud.
- Added comments to clarify the necessity of this change.


___



### **Changes walkthrough** 📝
<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Bug fix
</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>ci-tests.yml</strong><dd><code>Fix golangci-lint exit
code and add explanatory comments</code>&nbsp; </dd></summary>
<hr>

.github/workflows/ci-tests.yml
<li>Added comments to explain the need for
<code>issues-exit-code=0</code> in <br>golangci-lint step.<br> <li>
Changed <code>issues-exit-code</code> from 1 to 0 in golangci-lint
step.<br>


</details>
    

  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6369/files#diff-03609cb60b0c6e92fb771eb8787d6722b8c31ca4c03eabc788e147acd8c6fb43">+3/-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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants