Unicron add v3 e2e test coverage continue#4856
Conversation
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds many new Cypress v3/v4 end-to-end test suites and ~40 JSON Schema fixtures covering Projects, Signatures, CLA Manager, Events, Gerrits, GitHub (orgs/repos/login/redirect), Template, and related APIs; includes auth setup/teardown, data capture/reuse, positive/negative flows, conditional skips, and cleanup routines. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Cypress Test Runner
participant Store as Auth/Env Store
participant API as Backend API
participant Schema as JSON Schema Fixtures
rect rgb(240,248,255)
Note right of Runner: Global setup
Runner->>Store: retrieve bearer token, base URL, flags
Runner->>Schema: load/parse validation schemas
end
rect rgb(245,255,240)
Note right of Runner: Test execution (per suite)
Runner->>API: HTTP request (GET/POST/PUT/DELETE) with headers
API-->>Runner: response (2xx / 4xx / 5xx)
Runner->>Schema: validate response when 2xx (or per test rule)
alt resource created
Runner->>Runner: capture IDs (projectID, signatureID, gerritID...)
end
alt conditional accept non-200
Note left of Runner: Some tests accept 4xx/5xx per expectations
end
end
rect rgb(255,240,245)
Note right of Runner: Teardown
Runner->>API: DELETE captured resources (if any)
API-->>Runner: deletion response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/functional/cypress/e2e/v4/cla-manager.cy.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive test coverage for Project and Signatures V3 APIs by introducing new Cypress end-to-end test files and corresponding JSON schema fixtures for response validation.
- Adds 589 lines of test code for Signatures APIs covering both positive and negative test cases
- Adds 524 lines of test code for Project APIs with similar comprehensive coverage
- Creates 12 new JSON schema fixture files to validate API response structures
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| tests/functional/cypress/e2e/v3/signatures.cy.ts | New comprehensive test suite for Signatures V3 APIs including GET, POST, PUT operations with auth and validation tests |
| tests/functional/cypress/e2e/v3/project.cy.ts | New comprehensive test suite for Project V3 APIs with CRUD operations and error handling |
| tests/functional/cypress/fixtures/signatures/*.json | Six new JSON schema files for validating signature-related API responses |
| tests/functional/cypress/fixtures/project/*.json | Six new JSON schema files for validating project-related API responses |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/functional/cypress/fixtures/signatures/getEmployeeSignatures.json (1)
13-35: Consider adding required constraints to array items for robustness.The
signaturesarray items lack arequiredconstraint, making it possible for all properties to be optional. This differs fromgetSignatureById.jsonwhich enforces required fields. For consistency and test robustness, consider specifying which fields must be present in each signature object (at minimum:signatureID,projectID,signatureType)."signatures": { "type": "array", "items": { "type": "object", "properties": { "signatureID": { "type": "string" }, "userID": { "type": "string" }, "companyID": { "type": "string" }, "projectID": { "type": "string" }, "signatureType": { "type": "string" }, "employeeSignature": { "type": "boolean" } } + "required": ["signatureID", "projectID", "signatureType"] } }tests/functional/cypress/fixtures/project/getProjectsByExternalID.json (1)
13-27: Add required properties to projects array items.Similar to
getEmployeeSignatures.json, theprojectsarray items have norequiredconstraint. At minimum,projectIDshould be required in each project object. This improves schema robustness and test reliability."projects": { "type": "array", "items": { "type": "object", "properties": { "projectID": { "type": "string" }, "projectExternalID": { "type": "string" }, "projectName": { "type": "string" } } + "required": ["projectID"] } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
tests/functional/cypress/e2e/v3/project.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/signatures.cy.ts(1 hunks)tests/functional/cypress/fixtures/project/createProject.json(1 hunks)tests/functional/cypress/fixtures/project/getProjectById.json(1 hunks)tests/functional/cypress/fixtures/project/getProjectByName.json(1 hunks)tests/functional/cypress/fixtures/project/getProjects.json(1 hunks)tests/functional/cypress/fixtures/project/getProjectsByExternalID.json(1 hunks)tests/functional/cypress/fixtures/project/updateProject.json(1 hunks)tests/functional/cypress/fixtures/signatures/createSummaryReport.json(1 hunks)tests/functional/cypress/fixtures/signatures/getEmployeeSignatures.json(1 hunks)tests/functional/cypress/fixtures/signatures/getGitHubOrgWhitelist.json(1 hunks)tests/functional/cypress/fixtures/signatures/getSignatureById.json(1 hunks)tests/functional/cypress/fixtures/signatures/getUserSignatures.json(1 hunks)tests/functional/cypress/fixtures/signatures/updateApprovalList.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/functional/cypress/e2e/v3/project.cy.ts (1)
tests/functional/cypress/support/commands.js (9)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validateApiResponse(5-25)validate_204_Status(54-59)validate_expected_status(155-203)
tests/functional/cypress/e2e/v3/signatures.cy.ts (1)
tests/functional/cypress/support/commands.js (8)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validateApiResponse(5-25)validate_expected_status(155-203)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-lint
- GitHub Check: cypress-functional
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
tests/functional/cypress/e2e/v3/gerrits.cy.ts (2)
140-140: Inconsistent use offailOnStatusCodeparameter.Line 76 and line 99 use the
allowFailvariable forfailOnStatusCode, but this line uses a hardcodedfalse. For consistency, consider using theallowFailvariable throughout the test suite.Apply this diff for consistency:
- failOnStatusCode: false, // Allow various responses + failOnStatusCode: allowFail,
183-183: Inconsistent use offailOnStatusCodeparameter.Similar to line 140, this line uses a hardcoded
falsewhile other tests (lines 76, 99) use theallowFailvariable. For consistency, consider using theallowFailvariable.Apply this diff for consistency:
- failOnStatusCode: false, // Allow various responses + failOnStatusCode: allowFail,tests/functional/cypress/e2e/v3/events.cy.ts (3)
52-89: Reconsider the test classification and permissiveness.The section header states "EXPECT ONLY 2xx STATUS CODES," but the tests explicitly accept 400, 422, 404, and 5xx responses. This makes them neither true positive tests (which should assert success) nor focused negative tests (which should assert specific failures).
Accepting a wide range of status codes reduces the tests' ability to detect regressions. Consider:
- Splitting into separate positive tests (expecting 200 with valid data) and negative tests (expecting specific 4xx for edge cases)
- If the API behavior is truly unpredictable, document why in comments and consider raising an issue to stabilize the API contract
459-474: Replaceanytypes with specific types for better type safety.Using
anybypasses TypeScript's type checking. Consider defining an interface for request configurations or using inline object types.For example:
interface RequestConfig { method: string; url: string; acceptableStatuses?: number[]; expectedStatus?: number; expectedCode?: string | null; expectedMsg?: string | null; expectedMessageContains?: boolean; }Then replace
(req: any)with(req: RequestConfig).
501-521: Replaceanytypes with specific types (additional occurrences).Similar to the previous comment, these test blocks also use
anytypes. Apply the same type safety improvements here.Also applies to: 548-568, 599-624
tests/functional/cypress/e2e/v3/github-organizations.cy.ts (1)
33-33: Remove unused variable.The
localvariable is declared but never referenced in this test suite. Remove it to keep the code clean.Apply this diff:
- const local = Cypress.env('LOCAL');tests/functional/cypress/e2e/v3/github.cy.ts (1)
31-31: Remove unused variable.The
localvariable is declared but never used. Consider removing it for code cleanliness.Apply this diff:
- const local = Cypress.env('LOCAL');tests/functional/cypress/e2e/v3/github-repositories.cy.ts (1)
32-32: Remove unused variable.The
localvariable is declared but never used in this test suite.Apply this diff:
- const local = Cypress.env('LOCAL');tests/functional/cypress/fixtures/template/createCLAGroupTemplate.json (1)
5-10: Consider adding format validation for URL fields.The
individualPDFURLandcorporatePDFURLfields are defined as plain strings without format constraints. Adding"format": "uri"would provide basic validation that these fields contain well-formed URLs.Apply this diff to add URI format validation:
"properties": { "individualPDFURL": { - "type": "string" + "type": "string", + "format": "uri" }, "corporatePDFURL": { - "type": "string" + "type": "string", + "format": "uri" } },tests/functional/cypress/e2e/v3/template.cy.ts (1)
108-111: Usethis.skip()for conditional test execution.The test silently returns early without using Cypress's built-in skip mechanism. This makes it unclear in test reports whether the test passed or was intentionally skipped.
Apply this diff to properly skip the test:
if (!testTemplateID || !testClaGroupID) { cy.task('log', 'Skipping template creation - no test template or CLA group available'); + this.skip(); return; }Note: This pattern should be applied to other conditional skips in this file as well (lines 176-179, 209-212).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/events.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/gerrits.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/github-organizations.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/github-repositories.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/github.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/template.cy.ts(1 hunks)tests/functional/cypress/fixtures/cla-manager/addClaManager.json(1 hunks)tests/functional/cypress/fixtures/cla-manager/createClaManagerRequest.json(1 hunks)tests/functional/cypress/fixtures/cla-manager/getClaManagerRequest.json(1 hunks)tests/functional/cypress/fixtures/cla-manager/getClaManagerRequests.json(1 hunks)tests/functional/cypress/fixtures/gerrits/addGerrit.json(1 hunks)tests/functional/cypress/fixtures/gerrits/getGerritRepos.json(1 hunks)tests/functional/cypress/fixtures/github-organizations/addGithubOrganization.json(1 hunks)tests/functional/cypress/fixtures/github-organizations/updateGithubOrganizationConfig.json(1 hunks)tests/functional/cypress/fixtures/github-repositories/addGithubRepository.json(1 hunks)tests/functional/cypress/fixtures/github-repositories/getProjectGithubRepositories.json(1 hunks)tests/functional/cypress/fixtures/github/getGitHubOrgExists.json(1 hunks)tests/functional/cypress/fixtures/template/createCLAGroupTemplate.json(1 hunks)tests/functional/cypress/fixtures/template/getTemplates.json(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- tests/functional/cypress/fixtures/github/getGitHubOrgExists.json
- tests/functional/cypress/fixtures/github-repositories/getProjectGithubRepositories.json
- tests/functional/cypress/fixtures/github-organizations/addGithubOrganization.json
- tests/functional/cypress/fixtures/gerrits/addGerrit.json
- tests/functional/cypress/fixtures/gerrits/getGerritRepos.json
- tests/functional/cypress/fixtures/template/getTemplates.json
🧰 Additional context used
🧬 Code graph analysis (7)
tests/functional/cypress/e2e/v3/github.cy.ts (1)
tests/functional/cypress/support/commands.js (8)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)validate_expected_status(155-203)getXACLHeaders(273-285)validate_200_Status(47-52)validateApiResponse(5-25)
tests/functional/cypress/e2e/v3/template.cy.ts (1)
tests/functional/cypress/support/commands.js (7)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validateApiResponse(5-25)
tests/functional/cypress/e2e/v3/github-organizations.cy.ts (1)
tests/functional/cypress/support/commands.js (8)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validateApiResponse(5-25)validate_expected_status(155-203)
tests/functional/cypress/e2e/v3/github-repositories.cy.ts (1)
tests/functional/cypress/support/commands.js (8)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validateApiResponse(5-25)validate_expected_status(155-203)
tests/functional/cypress/e2e/v3/gerrits.cy.ts (1)
tests/functional/cypress/support/commands.js (7)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validateApiResponse(5-25)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts (1)
tests/functional/cypress/support/commands.js (8)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validateApiResponse(5-25)validate_expected_status(155-203)
tests/functional/cypress/e2e/v3/events.cy.ts (1)
tests/functional/cypress/support/commands.js (7)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validate_expected_status(155-203)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-lint
- GitHub Check: cypress-functional
🔇 Additional comments (14)
tests/functional/cypress/e2e/v3/gerrits.cy.ts (2)
163-167: Reconsider including 404 in expected error cases for POST.Including 404 as an expected error status when creating a gerrit seems unusual, since
testProjectIDwas just retrieved from a successful GET /project request. A 404 would indicate the project doesn't exist, which contradicts the prerequisite. While this might handle flaky test environments, it blurs the line between positive and negative test scenarios.Consider whether 404 is truly expected here, or if it indicates an environmental issue that should cause the test to fail.
271-271: Review inclusion of 401 status code.This test is authenticated (uses
bearerTokenon line 262-263), so expecting a 401 Unauthorized response alongside 404/422/204 seems inconsistent. If authentication is provided, 401 should not occur.Verify whether 401 is a realistic response for this authenticated DELETE request, or if it should be removed from the expected statuses.
tests/functional/cypress/e2e/v3/events.cy.ts (1)
477-522: The API does not validate UUID format; test accurately reflects actual behavior.After examining the codebase:
- The SearchEvents handler passes parameters directly to the service without UUID validation
- The repository accepts invalid UUIDs as literal string values and queries the database
- Invalid UUIDs return 200 with empty/filtered results—this is correct behavior, not a bug
- The UUID validation utility (
IsUUIDv4) exists but is not used in the SearchEvents pathThe test correctly accepts 200 because the API's design choice is to treat invalid UUIDs as search parameters rather than reject them. The
acceptableStatuses: [200, 400, 422]accurately reflects the API's actual behavior across different scenarios.Likely an incorrect or invalid review comment.
tests/functional/cypress/fixtures/github-organizations/updateGithubOrganizationConfig.json (1)
1-28: LGTM!The JSON schema is well-formed and appropriate for validating GitHub organization configuration updates. The flexible design with
additionalProperties: trueis suitable for test fixtures.tests/functional/cypress/e2e/v3/github-organizations.cy.ts (3)
46-70: LGTM! Excellent cleanup implementation.The before/after hooks properly manage test resources with appropriate token retrieval and cleanup logic. The conditional cleanup check prevents unnecessary API calls.
127-165: Well-designed flexible status handling.The test appropriately handles multiple valid outcomes (201 Created, 409 Conflict for existing resources, and 5xx server errors with test skipping). This is a good pattern for testing APIs in development.
243-271: Good coverage of authentication failures.The negative test comprehensively validates that all authenticated endpoints properly return 401 when called without a token. Using
cy.wrap().each()to iterate through test cases is an efficient pattern.tests/functional/cypress/e2e/v3/github.cy.ts (2)
46-70: Excellent OAuth flow testing!The test properly validates the OAuth redirect flow by checking for a 302 status, verifying the Location header exists, and confirming it points to GitHub's OAuth endpoint. The
followRedirect: falseoption is correctly used to test the redirect response itself.
72-103: Good practice skipping unreliable tests.Skipping tests that consistently return 500 errors is appropriate for a WIP PR. The skip reason is clearly documented, and the test logic is preserved for when the API is fully implemented.
tests/functional/cypress/fixtures/github-repositories/addGithubRepository.json (1)
1-40: LGTM!The JSON schema is well-structured with appropriate types for all repository properties. Using string types for
dateCreatedanddateModifiedis correct for validating ISO date string formats.tests/functional/cypress/e2e/v3/github-repositories.cy.ts (3)
38-46: Good use of random test data generation.Generating unique repository IDs using
Math.random()helps avoid test conflicts when tests run in parallel or repeatedly. This is a solid testing practice.
56-72: Excellent cleanup logic.The after hook properly cleans up created test resources, preventing test pollution. The conditional check ensures cleanup only runs when a repository was actually created.
300-348: Comprehensive invalid data testing.The test covers multiple invalid data scenarios including empty bodies, invalid fields, and malformed URLs. This thoroughness helps ensure robust API validation.
tests/functional/cypress/e2e/v3/template.cy.ts (1)
289-289: Error codes 602 and 604 are legitimate custom API error codes, not HTTP status codes.The review comment is based on a misconception. These codes are not being used as HTTP status codes. The test file clearly differentiates between HTTP status codes (
expectedStatuses: [400, 404, 422]) and custom API error codes (expectedCodes: ['400', '404', '602', '604']).Across the codebase, error code 602 is consistently used for body/payload validation errors, and error code 604 is used for path/query parameter validation errors. This pattern is evidenced in numerous test files (cla-manager.cy.ts, company.cy.ts, cla-group.cy.ts, projects.cy.ts, events.cy.ts, etc.), indicating intentional API design.
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts (1)
35-36: FixALLOW_FAILenv parsing so the toggle works as intended.The current comparison
Cypress.env('ALLOW_FAIL') === 1fails when the env var is set as a string ('1'or'true'), which is typical for environment variables. This keepsallowFailhard-coded and prevents running the suite in "allow failures" mode.As noted in the previous review, apply this diff to normalize the env value:
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const allowFailEnv = Cypress.env('ALLOW_FAIL'); + let allowFail: boolean = !( + allowFailEnv === 1 || + allowFailEnv === '1' || + allowFailEnv === true || + (typeof allowFailEnv === 'string' && allowFailEnv.toLowerCase() === 'true') + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts (1)
tests/functional/cypress/support/commands.js (8)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validateApiResponse(5-25)validate_expected_status(155-203)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-lint
- GitHub Check: cypress-functional
🔇 Additional comments (8)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts (8)
52-76: LGTM: Well-structured setup and cleanup hooks.The token retrieval and resource cleanup logic follows best practices for E2E tests.
82-126: LGTM: Setup tests properly initialize test data.The pattern for retrieving valid company and project IDs is well-structured and includes appropriate null checks.
191-227: LGTM: Flexible status handling for potentially flaky endpoint.The test correctly handles multiple response scenarios including server errors, 404s, and uses
this.skip()appropriately.
312-345: LGTM: DELETE test handles edge cases well.The test properly handles success (200/204), server errors, and 404 responses with appropriate conditional skipping.
352-399: LGTM: Comprehensive 401 testing across all endpoints.The test suite properly validates that all CLA Manager endpoints require authentication, with appropriate request configurations for each HTTP method.
401-452: LGTM: Malformed parameter validation is thorough.The test properly validates UUID format requirements and handles various response scenarios including lenient APIs.
510-568: LGTM: Non-existent resource handling is comprehensive.The test validates proper 4xx responses for non-existent companies, projects, requests, and managers, with appropriate fallbacks for lenient APIs.
470-476: Earlyreturnbreaks the Cypress command chain.Returning without a Cypress command inside
cy.wrap().each()returnsundefinedand can break the chain, potentially causing timing or execution issues.Apply this diff:
cy.wrap(requests).each((req: any) => { // Skip if we know these endpoints are problematic if (req.url.includes('/cla-manager/requests') || req.url.includes('/cla-manager')) { cy.task('log', `Skipping ${req.title} - endpoint known to have connection issues`); - return; + return cy.wrap(null); }Likely an incorrect or invalid review comment.
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts (2)
35-36: FixALLOW_FAILenv parsing so the toggle works correctly.This issue was previously identified:
Cypress.env('ALLOW_FAIL')returns a string (e.g.,'1'), so the strict equality check=== 1never matches. This prevents theallowFailtoggle from working as intended.Apply the suggested fix from the previous review to normalize the env value:
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const allowFailEnv = Cypress.env('ALLOW_FAIL'); + let allowFail: boolean = !( + allowFailEnv === 1 || + allowFailEnv === '1' || + allowFailEnv === true || + (typeof allowFailEnv === 'string' && allowFailEnv.toLowerCase() === 'true') + );
39-42: Correct type annotations for nullable variables.This issue was previously identified: variables are typed as
stringbut initialized tonull, causing TypeScript type mismatches.Apply this fix:
- let bearerToken: string = null; - let validCompanyID: string = null; - let validProjectID: string = null; - let createdRequestID: string = null; + let bearerToken: string | null = null; + let validCompanyID: string | null = null; + let validProjectID: string | null = null; + let createdRequestID: string | null = null;
🧹 Nitpick comments (3)
tests/functional/cypress/e2e/v3/github.cy.ts (1)
72-116: Consider simplifying the conditional logic.The test includes extensive branching to handle multiple response scenarios (2xx, 404, 4xx, 5xx). While this flexibility accommodates a WIP API, it may mask actual issues and makes the test harder to maintain. Once the API is stable, consider replacing the conditional logic with more specific assertions.
tests/functional/cypress/e2e/v3/cla-manager.cy.ts (2)
60-76: Consider adding cleanup for test CLA manager.The
after()hook only cleans up created CLA manager requests but doesn't clean up the CLA manager added at line 310-340. If the DELETE test at line 342-375 fails or is skipped, the test user (testUserLFID) will remain in the system.Consider adding this cleanup:
after(() => { + // Clean up test CLA manager + if (validCompanyID && validProjectID) { + cy.task('log', `Cleaning up test CLA manager: ${testUserLFID}`); + cy.request({ + method: 'DELETE', + url: `${claEndpoint}company/${validCompanyID}/project/${validProjectID}/cla-manager/${testUserLFID}`, + timeout: timeout, + failOnStatusCode: false, + headers: getXACLHeaders(), + auth: { + bearer: bearerToken, + }, + }).then((response) => { + cy.task('log', `Cleanup DELETE CLA manager ${testUserLFID}: ${response.status}`); + }); + } + if (createdRequestID && validCompanyID && validProjectID) { cy.task('log', `Cleaning up test CLA manager request: ${createdRequestID}`);
132-166: Multiple skipped tests indicate API stability concerns.Three tests are permanently skipped due to "consistent connection issues." While skipping unstable tests is pragmatic for keeping the test suite green, consider these alternatives:
- Conditional execution: Use environment variables to enable/disable these tests in different environments rather than hard-coding
.skip().- Retry logic: Implement retry mechanisms for flaky endpoints before skipping.
- Separate test suite: Move unstable tests to a dedicated "flaky" or "experimental" suite that runs independently.
- Monitoring: File issues/tickets tracking these API problems rather than silently skipping tests.
Example conditional skip:
const skipFlaky = Cypress.env('SKIP_FLAKY_TESTS'); (skipFlaky ? it.skip : it)('GET /company/.../cla-manager/requests - Get CLA Manager Requests', function () { // test implementation });Also applies to: 244-275, 277-308
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/github.cy.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts (1)
tests/functional/cypress/support/commands.js (8)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validate_expected_status(155-203)validateApiResponse(5-25)
tests/functional/cypress/e2e/v3/github.cy.ts (1)
tests/functional/cypress/support/commands.js (7)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)validate_expected_status(155-203)getXACLHeaders(273-285)validateApiResponse(5-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cypress-functional
- GitHub Check: build-test-lint
🔇 Additional comments (5)
tests/functional/cypress/e2e/v3/github.cy.ts (4)
46-70: Well-structured OAuth login test.The test correctly validates the OAuth flow initiation by checking for a 302 redirect and verifying the Location header points to GitHub's OAuth endpoint.
159-191: OAuth callback test appropriately handles various responses.The test correctly accommodates different valid OAuth callback behaviors (redirect or success page). The flexible status code handling is appropriate for the WIP stage.
198-225: Good coverage of unauthorized access scenarios.The test properly validates that authenticated endpoints return 401 when called without a bearer token, while gracefully handling unimplemented API responses.
227-339: Comprehensive negative test coverage with data-driven approach.The test suite effectively validates parameter validation and error handling using a data-driven pattern. The inclusion of multiple expected status codes accommodates the WIP state of the API while ensuring that error responses are properly validated.
tests/functional/cypress/e2e/v3/cla-manager.cy.ts (1)
1-600: Well-structured E2E test suite.The test organization is solid with clear separation between setup, positive cases, and negative cases. Good practices observed:
- Schema validation using
validateApiResponse()with JSON fixtures- Comprehensive negative testing (401, 4xx validation, non-existent resources)
- Appropriate error handling and logging for debugging
- Reusable test data structures
- Graceful handling of server errors with conditional skips
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/functional/cypress/e2e/v4/company.cy.ts (2)
878-891: Remove or differentiate duplicate test.This test has an identical title and tests the same endpoint as the test on lines 806-820. The duplicate test provides no additional coverage and should be removed, or if testing a different scenario, should have a distinct title.
975-976: Incorrect schema validation for contributor association endpoint.The schema
'company/getCompanyAdmins.json'is being used to validate the response from the contributor association endpoint, but this schema is for the admin list endpoint (line 936). Either use the correct schema for contributor association or remove this validation if the appropriate schema doesn't exist yet.tests/functional/cypress/e2e/v3/users.cy.ts (1)
17-18: Fix ALLOW_FAIL toggle so failOnStatusCode can be disabled
Cypress.env('ALLOW_FAIL')is delivered as a string when set via--env ALLOW_FAIL=1, so this comparison to the number1never matches. As a resultallowFailstaystrue, and every request keepsfailOnStatusCodeenabled even when the guard should relax. Update the parsing to accept the string form (and the boolean) so the opt-out works.- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const allowFailSetting = Cypress.env('ALLOW_FAIL'); + const allowFail = + !(allowFailSetting === true || + allowFailSetting === 1 || + allowFailSetting === '1');
♻️ Duplicate comments (17)
tests/functional/cypress/e2e/v3/github.cy.ts (2)
16-16: Remove unused variable.This variable is declared but never referenced in the test suite.
20-20: Fix type inconsistency for nullable token.The variable is typed as
stringbut initialized withnull, causing a TypeScript type mismatch.tests/functional/cypress/e2e/v3/template.cy.ts (1)
313-314: Pass log payloads via cy.task’s second argumentThis call is still pushing the body into the third parameter, but Cypress treats that slot as the options bag, so your payload is silently ignored. Please fold the data into the second argument (or stringify it) just as previously requested.
- cy.task('log', `Testing malformed ${req.method} ${req.url}`, req.body); + cy.task( + 'log', + `Testing malformed ${req.method} ${req.url} with body: ${JSON.stringify(req.body)}`, + );tests/functional/cypress/e2e/v3/signatures.cy.ts (2)
17-18: Fix ALLOW_FAIL toggle so failOnStatusCode can be disabledReiterating the earlier feedback: Cypress returns
'1'for this env var, so the numeric comparison never matches andfailOnStatusCodestays enabled. Normalize the value so the toggle works.- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const allowFailSetting = Cypress.env('ALLOW_FAIL'); + const allowFail = + !(allowFailSetting === true || + allowFailSetting === 1 || + allowFailSetting === '1');
238-238: Keep cy.task payloads in the second parameterThese log helpers still pass
reportData(and laterapprovalData/whitelistData) as the third argument, which Cypress interprets as the options object, so none of that diagnostic data actually reaches the task. Please fold the payload into the message (or pass it as the second argument) across this block.- cy.task('log', `Testing POST summary report for project ${validProjectID}`, reportData); + cy.task( + 'log', + `Testing POST summary report for project ${validProjectID}: ${JSON.stringify(reportData)}`, + );Apply the same pattern to the other
cy.taskcalls in this file that currently passapprovalDataorwhitelistDataas a third argument.tests/functional/cypress/e2e/v3/project.cy.ts (6)
17-17: ALLOW_FAIL env comparison requires normalizationThis issue was already identified in previous reviews. The environment variable comparison needs to handle string values.
109-133: Replace cy.skip() with this.skip()This issue was already identified in previous reviews. Cypress doesn't expose
cy.skip(), causing "cy.skip is not a function" errors.
135-158: Replace cy.skip() with this.skip()Same issue as Lines 109-133.
160-184: Replace cy.skip() with this.skip()Same issue as Lines 109-133.
230-235: Replace cy.skip() with this.skip()Same issue as Lines 109-133.
21-25: Type mismatch for nullable variablesVariables are typed as
stringbut initialized tonull, causing type errors. Similar issue flagged in cla-manager.cy.ts review.Apply this diff:
- let bearerToken: string = null; - let createdProjectID: string = null; - let testProjectID: string = null; - let testProjectSFID: string = null; - let testProjectName: string = null; + let bearerToken: string | null = null; + let createdProjectID: string | null = null; + let testProjectID: string | null = null; + let testProjectSFID: string | null = null; + let testProjectName: string | null = null;tests/functional/cypress/e2e/v3/cla-manager.cy.ts (3)
17-17: ALLOW_FAIL env parsing requires normalizationThis issue was already identified in previous reviews.
21-24: Type mismatch for nullable variablesThis issue was already identified in previous reviews.
470-475: Skip condition makes entire test block unreachableThis critical issue was already identified in previous reviews. The condition matches all URLs in this test block, rendering the tests ineffective.
tests/functional/cypress/e2e/v3/gerrits.cy.ts (3)
17-17: ALLOW_FAIL env parsing requires normalizationSame issue as in project.cy.ts and cla-manager.cy.ts.
357-379: Inconsistent expectedCodes type handlingThis issue was already identified in previous reviews. The
expectedCodesarrays use numbers here but strings elsewhere in the file.
21-23: Type mismatch for nullable variablesSame pattern as in other test files. Apply similar fix:
- let bearerToken: string = null; - let createdGerritID: string = null; - let testProjectID: string = null; + let bearerToken: string | null = null; + let createdGerritID: string | null = null; + let testProjectID: string | null = null;
🧹 Nitpick comments (3)
tests/functional/cypress/e2e/v3/github.cy.ts (1)
59-102: Consider inconsistency in 5xx error handling across tests.This test explicitly rejects 5xx errors (line 75), while the OAuth redirect test (lines 168-172) and negative tests (lines 204-209) accept 5xx errors with comments like "may not be implemented." If this API endpoint can legitimately return 5xx during development or when experiencing issues, the strict assertion may cause test brittleness.
If the strict 5xx rejection is intentional for this endpoint, consider adding a comment explaining why this endpoint should never return 5xx while others may.
tests/functional/cypress/e2e/v4/company.cy.ts (2)
782-785: Remove duplicate assignment.Line 782 and 785 both assign
list.companyExternalIDtocompanyExternalID. The second assignment is redundant.Apply this diff:
let list = response.body; companyExternalID = list.companyExternalID; companyID = list.companyID; signingEntityName = list.signingEntityName; - companyExternalID = list.companyExternalID; validateApiResponse('company/getCompanyByName.json', response);
224-236: Remove commented-out test case before final review.This commented-out test case for malformed SFID appears to be superseded by the active test immediately following it (lines 237-245). Since the PR is marked WIP, this can be addressed before final review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (30)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/company.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/docs.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/events.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/gerrits.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/github-organizations.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/github-repositories.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/github.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/health.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/organization.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/project.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/signatures.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/template.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/users.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/version.cy.ts(1 hunks)tests/functional/cypress/e2e/v4/cla-group.cy.ts(1 hunks)tests/functional/cypress/e2e/v4/cla-manager.cy.ts(1 hunks)tests/functional/cypress/e2e/v4/company.cy.ts(1 hunks)tests/functional/cypress/e2e/v4/docs.cy.ts(1 hunks)tests/functional/cypress/e2e/v4/events.cy.ts(1 hunks)tests/functional/cypress/e2e/v4/github-organizations.cy.ts(1 hunks)tests/functional/cypress/e2e/v4/github-repositories.cy.ts(1 hunks)tests/functional/cypress/e2e/v4/githubActivity.cy.ts(1 hunks)tests/functional/cypress/e2e/v4/gitlab-organizations.cy.ts(1 hunks)tests/functional/cypress/e2e/v4/gitlab-repositories.cy.ts(1 hunks)tests/functional/cypress/e2e/v4/health.cy.ts(1 hunks)tests/functional/cypress/e2e/v4/metrics.cy.ts(1 hunks)tests/functional/cypress/e2e/v4/projects.cy.ts(1 hunks)tests/functional/cypress/e2e/v4/signatures.cy.ts(1 hunks)tests/functional/cypress/e2e/v4/version.cy.ts(1 hunks)
✅ Files skipped from review due to trivial changes (18)
- tests/functional/cypress/e2e/v3/company.cy.ts
- tests/functional/cypress/e2e/v4/projects.cy.ts
- tests/functional/cypress/e2e/v4/cla-group.cy.ts
- tests/functional/cypress/e2e/v3/health.cy.ts
- tests/functional/cypress/e2e/v4/github-organizations.cy.ts
- tests/functional/cypress/e2e/v4/signatures.cy.ts
- tests/functional/cypress/e2e/v4/docs.cy.ts
- tests/functional/cypress/e2e/v4/githubActivity.cy.ts
- tests/functional/cypress/e2e/v4/version.cy.ts
- tests/functional/cypress/e2e/v4/gitlab-organizations.cy.ts
- tests/functional/cypress/e2e/v4/metrics.cy.ts
- tests/functional/cypress/e2e/v4/github-repositories.cy.ts
- tests/functional/cypress/e2e/v3/version.cy.ts
- tests/functional/cypress/e2e/v4/events.cy.ts
- tests/functional/cypress/e2e/v3/docs.cy.ts
- tests/functional/cypress/e2e/v4/gitlab-repositories.cy.ts
- tests/functional/cypress/e2e/v4/cla-manager.cy.ts
- tests/functional/cypress/e2e/v3/organization.cy.ts
🧰 Additional context used
🧬 Code graph analysis (9)
tests/functional/cypress/e2e/v3/template.cy.ts (1)
tests/functional/cypress/support/commands.js (7)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validateApiResponse(5-25)
tests/functional/cypress/e2e/v3/signatures.cy.ts (1)
tests/functional/cypress/support/commands.js (8)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validateApiResponse(5-25)validate_expected_status(155-203)
tests/functional/cypress/e2e/v3/gerrits.cy.ts (1)
tests/functional/cypress/support/commands.js (7)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validateApiResponse(5-25)
tests/functional/cypress/e2e/v3/github-repositories.cy.ts (1)
tests/functional/cypress/support/commands.js (8)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validateApiResponse(5-25)validate_expected_status(155-203)
tests/functional/cypress/e2e/v3/project.cy.ts (1)
tests/functional/cypress/support/commands.js (8)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validateApiResponse(5-25)validate_expected_status(155-203)
tests/functional/cypress/e2e/v3/github.cy.ts (1)
tests/functional/cypress/support/commands.js (7)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)validate_expected_status(155-203)getXACLHeaders(273-285)validateApiResponse(5-25)
tests/functional/cypress/e2e/v3/events.cy.ts (1)
tests/functional/cypress/support/commands.js (7)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validate_expected_status(155-203)
tests/functional/cypress/e2e/v3/github-organizations.cy.ts (1)
tests/functional/cypress/support/commands.js (8)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validateApiResponse(5-25)validate_expected_status(155-203)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts (1)
tests/functional/cypress/support/commands.js (8)
getAPIBaseURL(241-258)local(242-242)bearerToken(293-293)getTokenKey(294-330)getXACLHeaders(273-285)validate_200_Status(47-52)validate_expected_status(155-203)validateApiResponse(5-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-lint
- GitHub Check: cypress-functional
🔇 Additional comments (4)
tests/functional/cypress/e2e/v4/health.cy.ts (1)
1-2: License banner matches project standard.The SPDX MIT header aligns with the rest of the suite and introduces no functional changes. Nicely done.
tests/functional/cypress/e2e/v4/company.cy.ts (3)
1-3: LGTM! License header properly added.The copyright and SPDX license header follows standard format and is consistent with the PR objective to add license headers across test files.
80-767: Excellent comprehensive negative test coverage.The "Expected failures" section provides thorough coverage of authentication and validation errors across all Company API endpoints. The data-driven approach with local vs. remote environment handling is well-structured and makes the tests maintainable.
720-730: The test comment is factually accurate—verify whether this behavior is intentional by design.The verification confirms that
userIDis indeed not validated as a UUID format. In theCompanyCreateCompanyHandler(cla-backend-go/v2/company/handlers.go:345), theuserIDparameter is passed directly to the service layer without UUID validation. The only validation performed is an empty string check invalidateRequestCompanyAdmin()(line 1885). Although a UUID validator exists in the codebase (cla-backend-go/utils/validators.go:159), it is not applied to userID in this endpoint.This means the test correctly documents the actual API behavior. However, determine whether accepting any non-empty string for
userIDis by design or an oversight—if UUIDs are required, implement format validation in the handler.
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
More Cypress E2E V3 APIs test coverage:
project,signatures,events.Note that this is
WIP- I'll remove that label once ready for a review.cc @mlehotskylf @ahmedomosanya
Signed-off-by: Lukasz Gryglicki lgryglicki@cncf.io
Assisted by OpenAI
Assisted by GitHub Copilot