-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
chore: added one more step for limited-tests to contributor guidelines #34683
Conversation
WalkthroughThe update in Changes
Poem
Tip AI model upgrade
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
contributions/CodeContributionsGuidelines.md (4)
25-25
: Add a comma before "or".Use a comma before “or” if it connects two independent clauses (unless they are closely connected and short).
- 2. Branches are named as `fix/fix-name` or `feature/feature-name` + 2. Branches are named as `fix/fix-name`, or `feature/feature-name`Tools
LanguageTool
[uncategorized] ~25-~25: Use a comma before “or” if it connects two independent clauses (unless they are closely connected and short).
Context: ... 2. Branches are named asfix/fix-name
orfeature/feature-name
3. Please add te...(COMMA_COMPOUND_SENTENCE_3)
26-26
: Add a comma after "tests".Possible missing comma found.
- Client-side changes require Cypress/Jest tests while server-side changes require JUnit tests. + Client-side changes require Cypress/Jest tests, while server-side changes require JUnit tests.Tools
LanguageTool
[uncategorized] ~26-~26: Possible missing comma found.
Context: ...lient-side changes require Cypress/Jest tests while server-side changes require JUnit...(AI_HYDRA_LEO_MISSING_COMMA)
32-32
: Remove trailing space.Trailing spaces are not allowed.
- 9. If changes are requested, work on them, commit them back, and tag the reviewer again. + 9. If changes are requested, work on them, commit them back, and tag the reviewer again.Tools
Markdownlint
32-32: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
33-33
: Remove trailing space.Trailing spaces are not allowed.
- 10. Once all changes have been approved by the reviewer and the CI has run successfully, your PR will be merged into the base branch. Congratulations! + 10. Once all changes have been approved by the reviewer and the CI has run successfully, your PR will be merged into the base branch. Congratulations!Tools
Markdownlint
33-33: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- contributions/CodeContributionsGuidelines.md (1 hunks)
Additional context used
Path-based instructions (1)
contributions/CodeContributionsGuidelines.md (1)
Pattern
**/*.*
: Do not use cy.wait in code.
Do call the add function.
Do not keep duplicate lines in code.
Descriptive test names are used to clearly convey the intent of each test.
LanguageTool
contributions/CodeContributionsGuidelines.md
[uncategorized] ~25-~25: Use a comma before “or” if it connects two independent clauses (unless they are closely connected and short).
Context: ... 2. Branches are named asfix/fix-name
orfeature/feature-name
3. Please add te...(COMMA_COMPOUND_SENTENCE_3)
[uncategorized] ~26-~26: Possible missing comma found.
Context: ...lient-side changes require Cypress/Jest tests while server-side changes require JUnit...(AI_HYDRA_LEO_MISSING_COMMA)
Markdownlint
contributions/CodeContributionsGuidelines.md
32-32: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
33-33: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
Additional comments not posted (7)
contributions/CodeContributionsGuidelines.md (7)
27-27
: LGTM!The new step to add Cypress test paths to
limited-tests.txt
is a valuable addition to ensure proper tracking of tests.
28-28
: LGTM!The renumbering of the step for creating a pull request is logical and maintains the flow of instructions.
29-29
: LGTM!The renumbering of the step for calling out API changes in the pull request is logical and maintains the flow of instructions.
30-30
: LGTM!The renumbering of the step for linking the issue in the pull request is logical and maintains the flow of instructions.
31-31
: LGTM!The renumbering of the step for tagging the maintainer to start the build process is logical and maintains the flow of instructions.
32-32
: LGTM!The renumbering of the step for working on requested changes is logical and maintains the flow of instructions.
Tools
Markdownlint
32-32: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
33-33
: LGTM!The renumbering of the step for merging the pull request after approval is logical and maintains the flow of instructions.
Tools
Markdownlint
33-33: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
appsmithorg#34683) ## Description As we are have been seeing contributors are adding new cypress tests we want to execute it using `ci-test-limit`. We need to always make a change to limited-tests file to run new test now we will trigger `ci-test-limit` directly to save efforts. Fyi: we need to revert `limited-tests.txt` after everything looks good on PR. this step to reduce one step effort from our side 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.Sanity" ### 🔍 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/9804245011> > Commit: 8384096 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9804245011&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > <hr>Fri, 05 Jul 2024 07:03:52 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No
Description
As we are have been seeing contributors are adding new cypress tests we want to execute it using
ci-test-limit
. We need to always make a change to limited-tests file to run new test now we will triggerci-test-limit
directly to save efforts.Fyi: we need to revert
limited-tests.txt
after everything looks good on PR. this step to reduce one step effort from our sideFixes #
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.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9804245011
Commit: 8384096
Cypress dashboard.
Tags:
@tag.Sanity
Fri, 05 Jul 2024 07:03:52 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?