chore: Updated error handling #36288
Conversation
WalkthroughThe changes in this pull request improve the CI/CD workflow by enhancing the management of test results. A new mechanism for recording test statuses has been implemented through the introduction of a failure flag file. This facilitates clearer communication of test outcomes in pull requests. Additionally, the logic for evaluating test results has been centralized, ensuring that comments accurately reflect the actual status of the CI tests. Changes
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Additional context usedactionlint
Additional comments not posted (5)
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/build-client-server-count.yml (4 hunks)
- .github/workflows/ci-test-limited-with-count.yml (5 hunks)
Additional context used
actionlint
.github/workflows/ci-test-limited-with-count.yml
404-404: shellcheck reported issue in this script: SC2069:warning:2:22: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify)
(shellcheck)
.github/workflows/build-client-server-count.yml
273-273: shellcheck reported issue in this script: SC2002:style:1:23: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead
(shellcheck)
273-273: shellcheck reported issue in this script: SC2086:info:2:44: Double quote to prevent globbing and word splitting
(shellcheck)
279-279: shellcheck reported issue in this script: SC2086:info:5:37: Double quote to prevent globbing and word splitting
(shellcheck)
279-279: shellcheck reported issue in this script: SC2086:info:8:38: Double quote to prevent globbing and word splitting
(shellcheck)
279-279: shellcheck reported issue in this script: SC2086:info:12:38: Double quote to prevent globbing and word splitting
(shellcheck)
432-432: shellcheck reported issue in this script: SC2002:style:1:23: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead
(shellcheck)
432-432: shellcheck reported issue in this script: SC2086:info:2:44: Double quote to prevent globbing and word splitting
(shellcheck)
438-438: shellcheck reported issue in this script: SC2086:info:5:37: Double quote to prevent globbing and word splitting
(shellcheck)
438-438: shellcheck reported issue in this script: SC2086:info:8:38: Double quote to prevent globbing and word splitting
(shellcheck)
438-438: shellcheck reported issue in this script: SC2086:info:12:38: Double quote to prevent globbing and word splitting
(shellcheck)
Additional comments not posted (6)
.github/workflows/ci-test-limited-with-count.yml (4)
353-363: Great job on enhancing the test result handling!I really like how you've introduced a dedicated file to store the test status. This improves the clarity and makes it easier to determine the outcome of the tests.
The logic for checking the presence of "Total Failed: 0" in the summary file and setting the appropriate status is spot on. The code is well-structured, uses meaningful variable names, and includes helpful comments.
Keep up the excellent work!
365-365: Excellent addition for managing the cypress log files!Trimming the number of log files is a great practice to prevent the logs from consuming too much disk space over time. By keeping only the last 10 files, you strike a good balance between retaining enough history for debugging purposes and avoiding unnecessary clutter.
The
findcommand with the appropriate options is an efficient way to locate and delete the older log files. Well done!
377-383: Uploading the test status file as an artifact is a great idea!By uploading the
ci_test_status.txtfile as an artifact, you make it accessible to other jobs or steps in the workflow. This allows them to make decisions or perform actions based on the test outcome.Using the
actions/upload-artifact@v4action is the recommended approach for uploading artifacts in GitHub Actions. It's reliable and easy to use.Overwriting the artifact with the same name ensures that the latest status is always available, which is exactly what we want.
Nicely done!
386-386: Uploading the cypress console logs as an artifact is a valuable addition!Having access to the cypress console logs is crucial for debugging and investigating test failures. By uploading them as an artifact, you make it easy for developers to access and analyze the logs when needed.
Using the
actions/upload-artifact@v4action is the recommended approach for uploading artifacts in GitHub Actions. It's reliable and straightforward to use.Overwriting the artifact with the same name ensures that the latest logs are always available, which is helpful for troubleshooting.
The
if: always()condition is a smart choice. It guarantees that the logs are uploaded regardless of the success or failure of previous steps. This is especially important when tests fail, as the logs can provide valuable insights into what went wrong.Great work on this addition!
.github/workflows/build-client-server-count.yml (2)
264-269: Great job adding the step to download theci_test_statusartifact! 👍Downloading this artifact is essential for evaluating the CI test results in the following steps. Storing it in the
app/clientdirectory is a sensible choice.
277-292: Excellent work encapsulating the CI test result check in a separate step! 🙌Reading the
ci_test_status.txtfile and setting theci_test_failedenvironment variable based on its content is a clean and maintainable approach. It centralizes the test result evaluation and allows the subsequent steps to rely on theci_test_failedvariable.Handling the case when the file is not found by setting
ci_test_failedto "unknown" is a nice touch to ensure the workflow can proceed gracefully.Tools
actionlint
279-279: shellcheck reported issue in this script: SC2086:info:5:37: Double quote to prevent globbing and word splitting
(shellcheck)
279-279: shellcheck reported issue in this script: SC2086:info:8:38: Double quote to prevent globbing and word splitting
(shellcheck)
279-279: shellcheck reported issue in this script: SC2086:info:12:38: Double quote to prevent globbing and word splitting
(shellcheck)
github-actions
bot
added
the
skip-changelog
## Description Added extra checks for better reporting Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.ImportExport" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/10831607193> > Commit: 2a5049d > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10831607193&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.ImportExport` > Spec: > <hr>Thu, 12 Sep 2024 13:31:10 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced CI workflow to provide clearer test results through improved commenting on pull requests based on test outcomes. - Introduced a failure flag mechanism to explicitly record test execution status. - **Bug Fixes** - Improved reliability of CI process by ensuring important logs and results are preserved and uploaded even on test failures. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Added extra checks for better reporting
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.ImportExport"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10831607193
Commit: 2a5049d
Cypress dashboard.
Tags:
@tag.ImportExportSpec:
Thu, 12 Sep 2024 13:31:10 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes