Unicron v2 e2e test coverage #4874
Conversation
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
lukaszgryglicki
had a problem deploying
to
dev
GitHub Actions
Failure
lukaszgryglicki
had a problem deploying
to
dev
GitHub Actions
Failure
|
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. WalkthroughBackend null-safety fixes (safe company_name sorting, guarded email lowercasing, PynamoDB attribute query) and many new Cypress end-to-end test suites for v1/v2 plus support helpers updated to handle V2 token acquisition and varied V2 response formats. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Cypress Test
participant Commands as support/commands.js
participant Auth as Token Endpoint
participant API as Backend API
rect rgb(200,220,255)
Note over Client,Commands: V2 Token acquisition
Client->>Commands: getTokenForV2()
Commands->>Commands: check env TOKEN (not '-')
alt env TOKEN missing
Commands->>Auth: POST /auth/token
Auth-->>Commands: token
end
Commands->>Commands: store token (localStorage)
Commands-->>Client: wrapped token
end
rect rgb(220,255,220)
Note over Client,API: V2 response validation
Client->>API: request (uses token if needed)
API-->>Client: response (string/object/errors)
Client->>Commands: validate_expected_status(response)
Commands->>Commands: extract code/message from body (handles string, {"404":...}, {"errors":...})
Commands-->>Client: assertion pass/fail
end
sequenceDiagram
participant Caller as Backend caller
participant CompanyCtrl as company.get_companies
participant DB as Backend DB
rect rgb(240,240,255)
Note over Caller,CompanyCtrl: Sorting null-safety change
Caller->>CompanyCtrl: get_companies()
CompanyCtrl->>DB: fetch companies
DB-->>CompanyCtrl: [{company_name: null}, {...}]
CompanyCtrl->>CompanyCtrl: sort by (i.get('company_name') or '')
CompanyCtrl-->>Caller: sorted result (no exception)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (20)
cla-backend/cla/controllers/user.py (1)
498-503: Robust handling ofauth_user.emailis correct; consider observability for placeholder usageGuarding the
.lower()call and falling back to a placeholder derived fromauth_user.usernameprevents failures whenNoneand guarantees a non-emptylf_email, which matches the intent described in the PR. You might optionally add a debug log when the placeholder path is taken or centralize the placeholder domain in config, but the current logic is sound.cla-backend/cla/controllers/event.py (2)
99-99: Consider parameterizing the PII flag.The
contains_piiflag is hardcoded toFalse. Depending on the event types and data being stored, some events may contain personally identifiable information. Consider adding an optional parameter to allow callers to specify this flag when appropriate.def create_event( response=None, event_type=None, event_company_id=None, event_data=None, event_project_id=None, user_id=None, + contains_pii=False, ):Then use the parameter:
- event.set_event_date_and_contains_pii(contains_pii=False) + event.set_event_date_and_contains_pii(contains_pii=contains_pii)
103-105: Refine exception handling for better debuggability.While catching broad
Exceptionprevents stack traces from leaking to clients, it makes debugging difficult and masks specific error conditions. Consider catching specific exceptions and adding logging.+ from cla.models import ValidationError + import logging + + logger = logging.getLogger(__name__) + try: event = get_event_instance() # ... rest of function ... - except Exception as err: + except (DoesNotExist, ValidationError) as err: + logger.warning(f"Event creation validation error: {err}") response.status = HTTP_400 return {"errors": {"create_event": str(err)}} + except Exception as err: + logger.error(f"Unexpected error creating event: {err}", exc_info=True) + response.status = HTTP_400 + return {"errors": {"create_event": "An unexpected error occurred"}}tests/functional/cypress/e2e/v2/health.cy.ts (1)
6-9: Good test suite setup, but theallowFailnaming is confusing.The variable name
allowFailis semantically inverted from its usage. WhenALLOW_FAIL=1, the variable becomesfalse, which disablesfailOnStatusCode. Consider renaming tofailOnStatusCodedirectly for clarity:- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const failOnStatusCode: boolean = Cypress.env('ALLOW_FAIL') !== 1;This would make the intent clearer when used in requests.
tests/functional/cypress/e2e/v1/signature.cy.ts (1)
4-10: Remove unused importvalidate_401_Status.The
validate_401_Statusfunction is imported but not used in this file. The 401 validation in the "Expected failures" section uses manual assertions instead.import { validate_200_Status, - validate_401_Status, validate_expected_status, getAPIBaseURL, getTokenForV2, } from '../../support/commands';tests/functional/cypress/e2e/v2/company.cy.ts (2)
11-12: Useconstfor immutable test data.
validCompanyIDis never reassigned, so it should be declared withconstfor clarity and to prevent accidental modification.- let validCompanyID = '550e8400-e29b-41d4-a716-446655440000'; + const validCompanyID = '550e8400-e29b-41d4-a716-446655440000';
47-91: Reduce code duplication in conditional branches.The request logic for fetching a company by ID is duplicated in both the
if (companyWithName)andelsebranches (lines 52-63 and 67-78). Consider extracting the company ID selection and making a single request:if (companiesResponse.body.length > 0) { - // Use the first company ID that has a company_name const companyWithName = companiesResponse.body.find((c) => c.company_name !== null); - if (companyWithName) { - const companyId = companyWithName.company_id; - cy.request({ - method: 'GET', - url: `${claEndpoint}company/${companyId}`, - timeout: timeout, - failOnStatusCode: allowFail, - }).then((response) => { - return cy.logJson('GET /company/{company_id} response', response).then(() => { - validate_200_Status(response); - expect(response.body).to.be.an('object'); - expect(response.body).to.have.property('company_id'); - }); - }); - } else { - // Use any company ID to test the endpoint - const companyId = companiesResponse.body[0].company_id; - cy.request({ - method: 'GET', - url: `${claEndpoint}company/${companyId}`, - timeout: timeout, - failOnStatusCode: allowFail, - }).then((response) => { - return cy.logJson('GET /company/{company_id} response', response).then(() => { - validate_200_Status(response); - expect(response.body).to.be.an('object'); - expect(response.body).to.have.property('company_id'); - }); - }); - } + const companyId = companyWithName?.company_id ?? companiesResponse.body[0].company_id; + cy.request({ + method: 'GET', + url: `${claEndpoint}company/${companyId}`, + timeout: timeout, + failOnStatusCode: allowFail, + }).then((response) => { + return cy.logJson('GET /company/{company_id} response', response).then(() => { + validate_200_Status(response); + expect(response.body).to.be.an('object'); + expect(response.body).to.have.property('company_id'); + }); + }); } else {tests/functional/cypress/e2e/v2/project.cy.ts (2)
64-75: Clarify type assertions for response body.The assertion on line 67 checks that
response.bodyis an object, but line 72 then expects it to be an array. In JavaScript, arrays are objects, so this works technically, but the logic is confusing. Consider restructuring for clarity:- validate_200_Status(response); - expect(response.body).to.be.an('object'); - // V2 API returns error object when project not found, array when project exists - if (response.body.errors) { - expect(response.body).to.have.property('errors'); - } else { - expect(response.body).to.be.an('array'); - } + validate_200_Status(response); + // V2 API returns error object when project not found, array when project exists + if (Array.isArray(response.body)) { + // Companies returned as array + } else { + expect(response.body).to.be.an('object'); + expect(response.body).to.have.property('errors'); + }
118-124: Test case may be misplaced in project test suite.The test case for
POST /user/{user_id}/request-company-whitelist/{company_id}appears to be a user-related endpoint rather than a project endpoint. Consider moving this to a dedicated user test file (e.g.,v2/user.cy.ts) for better organization.tests/functional/cypress/e2e/v1/company.cy.ts (1)
4-10: Remove unused importvalidate_401_Status.The
validate_401_Statusfunction is imported but not used anywhere in this file.import { validate_200_Status, - validate_401_Status, validate_expected_status, getAPIBaseURL, getTokenForV2, } from '../../support/commands';tests/functional/cypress/e2e/v1/project.cy.ts (2)
4-10: Remove unused importvalidate_401_Status.Same as other V1 test files -
validate_401_Statusis imported but not used.import { validate_200_Status, - validate_401_Status, validate_expected_status, getAPIBaseURL, getTokenForV2, } from '../../support/commands';
29-32: Remove unused test data variable.
validProjectSFDCIDis defined but never used in any test case.// Test data const validProjectID = '550e8400-e29b-41d4-a716-446655440000'; - const validProjectSFDCID = 'a096s00000003ZFmAAM'; const validExternalProjectID = 'test-project-external';tests/functional/cypress/e2e/v2/repository.cy.ts (1)
11-19: Multiple test data variables are unused in active tests.Several test data constants (
validInstallationID,validRepoID,validChangeRequestID,validGitlabRepoID,validMergeRequestID) are only used in skipped tests. This is acceptable if the skipped tests will be enabled later, but consider adding a comment indicating these are reserved for future tests.tests/functional/cypress/e2e/v2/user.cy.ts (2)
4-10: Unused import:validate_401_StatusThe
validate_401_Statusfunction is imported but never used in this file. The 401 validation in lines 117-121 uses direct assertions instead.Either remove the unused import or use the helper for consistency:
-import { validate_200_Status, validate_expected_status, getAPIBaseURL, getTokenForV2 } from '../../support/commands'; +import { validate_200_Status, validate_expected_status, getAPIBaseURL, getTokenForV2 } from '../../support/commands';If keeping the import, consider using it in the 401 test (lines 117-121) for consistency with other test files.
32-52: Contradictory test logic: expects 200 but accepts error responsesThe test calls
validate_200_Status(response)which asserts HTTP 200, then checks ifresponse.body.errorsexists. If the API returns an error object with HTTP 200, this is an API design issue worth noting. If the API returns a non-200 for user not found, the test will fail.Consider clarifying the expected behavior:
}).then((response) => { return cy.logJson('GET /user/{user_id} response', response).then(() => { - validate_200_Status(response); - expect(response.body).to.be.an('object'); - // V2 API can return user data or error object - if (response.body.errors) { - // API returned error (user not found), which is valid - expect(response.body).to.have.property('errors'); - } else { - // API returned user data - expect(response.body).to.have.property('user_id'); - } + // API returns 200 for both found and not-found cases + validate_200_Status(response); + expect(response.body).to.be.an('object'); + // Note: Using synthetic UUID, expect either user data or error object }); });tests/functional/cypress/e2e/v1/events.cy.ts (2)
4-10: Unused import:validate_401_StatusSimilar to other files,
validate_401_Statusis imported but not used. The skipped 401 test at line 130 uses direct assertions instead.Remove the unused import for cleaner code:
import { validate_200_Status, - validate_401_Status, validate_expected_status, getAPIBaseURL, getTokenForV2, } from '../../support/commands';
39-124: All tests are skipped - consider usingdescribe.skipor documenting planEvery test in this file is marked with
it.skip, which means no actual test coverage is being added. While the skip reasons are documented (large responses, ProvisionedThroughputExceededException, inconsistent behavior), consider:
- Using
describe.skipat the top level for cleaner intent- Adding a TODO comment or issue reference for when these should be enabled
-describe('To Validate & test Events APIs via API call (V1)', function () { +// TODO: Enable tests when V1 Events API stabilizes (see issue #XXX) +describe.skip('To Validate & test Events APIs via API call (V1)', function () {tests/functional/cypress/support/commands.js (1)
239-265: Code duplication withgetTokenForV3
getTokenForV2(lines 239-265) is nearly identical togetTokenForV3(lines 267-294). The only differences are:
- Log messages mention "V2" vs "V3"
getTokenForV2returnscy.wrap(token)whilegetTokenForV3returnstokendirectlyConsider consolidating into a single parameterized function:
-export function getTokenForV2() { - // V2 APIs use the same token generation as V3/V4 - cy.task('log', '--> getting token by request for V2'); - return cy - .request({ - // ... identical body ... - }) - .then((response) => { - expect(response.status).to.eq(200); - const token = response.body.access_token; - cy.task('log', `--> got token ${shortenMiddle(token)} from request for V2`); - return cy.wrap(token); - }); -} +function getTokenForVersion(version) { + cy.task('log', `--> getting token by request for ${version}`); + return cy + .request({ + method: 'POST', + url: Cypress.env('AUTH0_TOKEN_API'), + headers: { 'content-type': 'application/json' }, + body: { + grant_type: 'http://auth0.com/oauth/grant-type/password-realm', + realm: 'Username-Password-Authentication', + username: Cypress.env('AUTH0_USER_NAME'), + password: Cypress.env('AUTH0_PASSWORD'), + client_id: Cypress.env('AUTH0_CLIENT_ID'), + audience: 'https://api-gw.dev.platform.linuxfoundation.org/', + scope: 'access:api openid profile email', + }, + }) + .then((response) => { + expect(response.status).to.eq(200); + const token = response.body.access_token; + cy.task('log', `--> got token ${shortenMiddle(token)} from request for ${version}`); + return cy.wrap(token); + }); +} + +export function getTokenForV2() { + return getTokenForVersion('V2'); +} + +export function getTokenForV3() { + return getTokenForVersion('V3'); +}Note: This would change
getTokenForV3to also usecy.wrap(token). Verify this doesn't break existing V3 tests.tests/functional/cypress/e2e/v1/user.cy.ts (1)
4-10: Unused import:validate_401_StatusSame issue as other files -
validate_401_Statusis imported but the 401 test (lines 141-160) uses direct assertions instead.Remove the unused import:
import { validate_200_Status, - validate_401_Status, validate_expected_status, getAPIBaseURL, getTokenForV2, } from '../../support/commands';tests/functional/cypress/e2e/v2/events.cy.ts (1)
4-10: Unused import:validate_401_StatusSame pattern as other files -
validate_401_Statusis imported but the 401 test uses direct assertions instead.Remove the unused import for consistency:
import { validate_200_Status, - validate_401_Status, validate_expected_status, getAPIBaseURL, getTokenForV2, } from '../../support/commands';
📜 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 (21)
cla-backend/cla/controllers/company.py(2 hunks)cla-backend/cla/controllers/event.py(1 hunks)cla-backend/cla/controllers/user.py(1 hunks)cla-backend/cla/models/dynamo_models.py(1 hunks)tests/functional/cypress/e2e/v1/company.cy.ts(1 hunks)tests/functional/cypress/e2e/v1/events.cy.ts(1 hunks)tests/functional/cypress/e2e/v1/gerrit.cy.ts(1 hunks)tests/functional/cypress/e2e/v1/project.cy.ts(1 hunks)tests/functional/cypress/e2e/v1/repository.cy.ts(1 hunks)tests/functional/cypress/e2e/v1/signature.cy.ts(1 hunks)tests/functional/cypress/e2e/v1/user.cy.ts(1 hunks)tests/functional/cypress/e2e/v2/company.cy.ts(1 hunks)tests/functional/cypress/e2e/v2/events.cy.ts(1 hunks)tests/functional/cypress/e2e/v2/gerrit.cy.ts(1 hunks)tests/functional/cypress/e2e/v2/github.cy.ts(1 hunks)tests/functional/cypress/e2e/v2/health.cy.ts(1 hunks)tests/functional/cypress/e2e/v2/project.cy.ts(1 hunks)tests/functional/cypress/e2e/v2/repository.cy.ts(1 hunks)tests/functional/cypress/e2e/v2/signatures.cy.ts(1 hunks)tests/functional/cypress/e2e/v2/user.cy.ts(1 hunks)tests/functional/cypress/support/commands.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (15)
tests/functional/cypress/e2e/v1/events.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(296-323)bearerToken(358-358)envToken(361-361)getTokenForV2(239-265)validate_200_Status(47-52)validate_expected_status(155-230)
tests/functional/cypress/e2e/v1/company.cy.ts (1)
tests/functional/cypress/support/commands.js (4)
getAPIBaseURL(296-323)getTokenForV2(239-265)validate_200_Status(47-52)validate_expected_status(155-230)
tests/functional/cypress/e2e/v2/company.cy.ts (1)
tests/functional/cypress/support/commands.js (3)
getAPIBaseURL(296-323)validate_200_Status(47-52)validate_expected_status(155-230)
tests/functional/cypress/e2e/v1/signature.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(296-323)bearerToken(358-358)envToken(361-361)getTokenForV2(239-265)validate_200_Status(47-52)validate_expected_status(155-230)
tests/functional/cypress/e2e/v2/project.cy.ts (1)
tests/functional/cypress/support/commands.js (3)
getAPIBaseURL(296-323)validate_200_Status(47-52)validate_expected_status(155-230)
cla-backend/cla/controllers/user.py (3)
cla-backend/cla/tests/unit/test_company_event.py (1)
auth_user(20-23)cla-backend/cla/models/dynamo_models.py (1)
set_lf_email(1846-1847)cla-backend/cla/models/model_interfaces.py (1)
set_lf_email(445-446)
cla-backend/cla/controllers/event.py (2)
cla-backend/cla/utils.py (1)
get_event_instance(297-306)cla-backend/cla/models/dynamo_models.py (1)
set_event_date_and_contains_pii(5202-5206)
tests/functional/cypress/e2e/v2/signatures.cy.ts (1)
tests/functional/cypress/support/commands.js (3)
getAPIBaseURL(296-323)validate_200_Status(47-52)validate_expected_status(155-230)
tests/functional/cypress/e2e/v1/user.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(296-323)bearerToken(358-358)envToken(361-361)getTokenForV2(239-265)validate_200_Status(47-52)validate_expected_status(155-230)
tests/functional/cypress/e2e/v2/github.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(296-323)bearerToken(358-358)envToken(361-361)getTokenForV2(239-265)validate_200_Status(47-52)validate_expected_status(155-230)
tests/functional/cypress/e2e/v2/health.cy.ts (1)
tests/functional/cypress/support/commands.js (3)
getAPIBaseURL(296-323)validate_200_Status(47-52)validate_expected_status(155-230)
tests/functional/cypress/e2e/v2/events.cy.ts (1)
tests/functional/cypress/support/commands.js (4)
getAPIBaseURL(296-323)getTokenForV2(239-265)validate_200_Status(47-52)validate_expected_status(155-230)
tests/functional/cypress/e2e/v2/user.cy.ts (1)
tests/functional/cypress/support/commands.js (16)
getAPIBaseURL(296-323)getTokenForV2(239-265)validate_200_Status(47-52)body(31-31)body(50-50)body(57-57)body(66-66)body(78-78)body(90-90)body(104-104)body(114-114)body(124-124)body(136-136)body(148-148)body(188-188)validate_expected_status(155-230)
tests/functional/cypress/e2e/v2/gerrit.cy.ts (1)
tests/functional/cypress/support/commands.js (3)
getAPIBaseURL(296-323)validate_200_Status(47-52)validate_expected_status(155-230)
tests/functional/cypress/e2e/v1/repository.cy.ts (1)
tests/functional/cypress/support/commands.js (5)
getAPIBaseURL(296-323)bearerToken(358-358)getTokenForV2(239-265)validate_200_Status(47-52)validate_expected_status(155-230)
🪛 Ruff (0.14.5)
cla-backend/cla/controllers/event.py
103-103: Do not catch blind exception: Exception
(BLE001)
⏰ 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 (30)
cla-backend/cla/controllers/company.py (1)
33-33: Safer sorting for missingcompany_namevalues looks goodUsing
(i.get('company_name') or '').casefold()avoids KeyErrors and gracefully handlesNone/missing names while preserving case-insensitive ordering. No further changes needed.Also applies to: 49-49
cla-backend/cla/models/dynamo_models.py (1)
2264-2264: Filter-expression scan change is correct and low-riskSwitching to
UserModel.user_company_id == str(company_id)for the scan filter keeps behavior the same while aligning with expression-based query style. No issues spotted.tests/functional/cypress/e2e/v2/health.cy.ts (1)
15-93: LGTM!The test structure is well-organized with clear separation between positive and negative test cases. The parametrized approach for expected failures using
cy.wrap().each()is a good pattern for maintainability.tests/functional/cypress/e2e/v1/signature.cy.ts (2)
39-55: Assertion may conflict with expected API behavior.The comment on line 52 states "V1 API can return signature data or error object - both are valid," but
validate_200_Statuswill fail if the API returns a non-200 status code (e.g., 404 for non-existent signatures). If error responses are acceptable, consider using a more flexible assertion:- validate_200_Status(response); - expect(response.body).to.be.an('object'); - // V1 API can return signature data or error object - both are valid + // V1 API can return signature data (200) or error (404) for test UUIDs + expect([200, 404]).to.include(response.status); + expect(response.body).to.be.an('object');Alternatively, if only 200 responses are expected, update the comment to reflect this.
91-196: LGTM!The expected failures section is well-structured with clear separation between 401 authentication tests and 4xx validation tests. The parametrized approach allows easy extension of test cases.
tests/functional/cypress/e2e/v2/company.cy.ts (1)
98-162: LGTM!The expected failures section correctly validates that POST, PUT, and DELETE methods are not defined for V2 Company endpoints, using appropriate 404 status expectations with partial message matching.
tests/functional/cypress/e2e/v1/company.cy.ts (1)
38-92: All positive tests are skipped.The entire positive test suite is skipped. While the comments provide valid reasons (environment-specific issues), consider tracking these as TODO items or creating issues to enable these tests once the V1 Company API behavior is stabilized or test data is available.
tests/functional/cypress/e2e/v2/repository.cy.ts (1)
80-161: LGTM!The active test correctly uses a flexible assertion to handle both success (200) and validation error (400) responses. The expected failures section properly validates 4xx/405 responses for invalid providers and disallowed methods.
tests/functional/cypress/e2e/v2/user.cy.ts (2)
6-21: LGTM - Well-structured test setupThe token acquisition logic correctly handles both environment-provided tokens and dynamic token retrieval. The guard for placeholder token value (
'-') is a good pattern for CI flexibility.
101-124: LGTM - Good 401 unauthorized test coverageThe iteration pattern using
cy.wrap().each()is appropriate for testing multiple endpoints. The assertions correctly validate both status code and response body format.tests/functional/cypress/support/commands.js (3)
190-215: LGTM - Robust V2 response format handlingThe cascading logic correctly handles multiple V2 API response formats:
- String responses with quotes stripped
{"404": "..."}format{"errors": {"key": "value"}}and{"errors": {"405 Method Not Allowed": null}}formatsThe defensive fallback to empty string (line 223) prevents assertion failures on undefined messages.
310-319: LGTM - V1/V2 API base URL supportThe new cases follow the established pattern for V3/V4. Local environment correctly uses port 5000, and remote URLs are properly configured for the dev platform.
362-362: LGTM - Placeholder token handlingThe
!== '-'check allows CI to pass a placeholder value to force dynamic token retrieval, which is a useful pattern for test flexibility.tests/functional/cypress/e2e/v1/user.cy.ts (2)
121-161: LGTM - Good 401 test coverageThe iteration pattern and assertions are consistent with other test files. The test correctly validates both HTTP status and response body format for unauthorized requests.
38-61: Endpoint design suggests concern is mitigated; verify idempotencyThe test already accounts for this scenario: the "Create/get" endpoint name pattern and the test comment "V1 API can return user data or error object - both are valid" (line 58) indicate the endpoint handles repeated calls with the same data. The same hardcoded email is also already used across v3 tests without reported issues.
However, verify that the POST /user/gerrit endpoint is truly idempotent or that your test environment has cleanup mechanisms. If confirmed, this pattern is safe and no changes are needed.
tests/functional/cypress/e2e/v2/events.cy.ts (2)
65-82: LGTM - Well-structured cache clear testThe test correctly validates the 200 status, object body, and specific
{status: 'OK'}response format.
88-128: LGTM - Comprehensive 401 unauthorized coverageThe test covers both V2 Events endpoints without authentication and validates the expected 401 response format consistently.
tests/functional/cypress/e2e/v2/gerrit.cy.ts (3)
1-13: Well-structured test setup.The test configuration, imports, and test data are appropriately defined. The structure aligns with other V2 test suites in this PR.
19-47: Positive test cases are well-implemented.Both test cases appropriately validate successful responses with clear assertions and helpful logging.
52-114: Comprehensive error scenario coverage.The data-driven test approach effectively validates multiple error conditions. Good documentation of V2 API behavior (e.g., line 74 noting that invalid contract types return 200 with error in body).
tests/functional/cypress/e2e/v1/gerrit.cy.ts (2)
1-31: Authentication setup is correctly implemented.The before hook properly handles token acquisition from environment or via
getTokenForV2(), consistent with other V1 test suites.
37-140: Good documentation of skipped tests.The comments clearly explain why tests are skipped (server errors in development, inconsistent V1 API behavior). This documentation will be helpful when enabling these tests in the future.
tests/functional/cypress/e2e/v1/repository.cy.ts (3)
36-52: Authenticated endpoint test is correctly implemented.The test properly includes the Authorization header and validates the response appropriately.
57-87: Authentication validation test is well-structured.Properly validates 401 behavior when authentication is missing, with comprehensive assertions on status, statusText, and response body.
89-140: Comprehensive validation error coverage.The test cases cover invalid UUIDs and method restrictions appropriately. Line 105's comment usefully documents that V1 accepts invalid UUIDs and returns errors in the response body rather than rejecting at validation.
tests/functional/cypress/e2e/v2/signatures.cy.ts (2)
21-102: Signature endpoint tests handle various scenarios correctly.The tests appropriately cover multiple signature request types. Line 95's handling of potential redirects with
failOnStatusCode: falseand the status validation on line 99 correctly accommodate the endpoint's redirect behavior.
107-184: Error scenario coverage is thorough.The test cases appropriately validate missing parameters, invalid UUIDs, and unsupported HTTP methods across signature endpoints.
tests/functional/cypress/e2e/v2/github.cy.ts (3)
33-73: OAuth and webhook tests appropriately handle various response scenarios.Line 47 correctly accepts multiple status codes (200, 302, 400) for OAuth callback behavior, and the webhook test validates response structure appropriately.
75-90: Excellent documentation of skipped test.The detailed comment (lines 76-77) explaining the specific backend error (
GithubException.__init__()issue) will be invaluable when re-enabling this test after the backend fix.
95-209: Comprehensive error scenario validation.The test cases cover authentication failures, invalid providers, malformed data, and method restrictions across GitHub endpoints, providing thorough negative path coverage.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive Cypress E2E test coverage for V1 and V2 APIs (titled "Unicron v2"), implementing test suites for various API endpoints including user, signature, project, company, events, GitHub, Gerrit, repository, and health endpoints. The changes also include backend improvements for null-safe handling in company sorting, user email validation, and PynamoDB query syntax updates.
Reviewed changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/functional/cypress/support/commands.js | Enhanced error parsing for V2 API responses, added V2 token generation function, V1/V2 endpoint support, and token validation logic |
| tests/functional/cypress/e2e/v2/*.cy.ts | Nine new test files covering V2 API endpoints (user, signatures, repository, project, health, github, gerrit, events, company) |
| tests/functional/cypress/e2e/v1/*.cy.ts | Seven new test files covering V1 API endpoints (user, signature, repository, project, gerrit, events, company) |
| cla-backend/cla/models/dynamo_models.py | Updated PynamoDB scan syntax from deprecated keyword-argument style to conditional expression |
| cla-backend/cla/controllers/user.py | Added null-safe email handling for user creation |
| cla-backend/cla/controllers/event.py | New create_event function for event creation |
| cla-backend/cla/controllers/company.py | Null-safe sorting for company_name field |
Comments suppressed due to low confidence (7)
tests/functional/cypress/e2e/v1/company.cy.ts:10
- Unused import validate_401_Status.
import {
validate_200_Status,
validate_401_Status,
validate_expected_status,
getAPIBaseURL,
getTokenForV2,
} from '../../support/commands';
tests/functional/cypress/e2e/v1/gerrit.cy.ts:10
- Unused import validate_401_Status.
import {
validate_200_Status,
validate_401_Status,
validate_expected_status,
getAPIBaseURL,
getTokenForV2,
} from '../../support/commands';
tests/functional/cypress/e2e/v1/project.cy.ts:10
- Unused import validate_401_Status.
import {
validate_200_Status,
validate_401_Status,
validate_expected_status,
getAPIBaseURL,
getTokenForV2,
} from '../../support/commands';
tests/functional/cypress/e2e/v1/repository.cy.ts:10
- Unused import validate_401_Status.
import {
validate_200_Status,
validate_401_Status,
validate_expected_status,
getAPIBaseURL,
getTokenForV2,
} from '../../support/commands';
tests/functional/cypress/e2e/v1/signature.cy.ts:10
- Unused import validate_401_Status.
import {
validate_200_Status,
validate_401_Status,
validate_expected_status,
getAPIBaseURL,
getTokenForV2,
} from '../../support/commands';
tests/functional/cypress/e2e/v1/user.cy.ts:10
- Unused import validate_401_Status.
import {
validate_200_Status,
validate_401_Status,
validate_expected_status,
getAPIBaseURL,
getTokenForV2,
} from '../../support/commands';
tests/functional/cypress/e2e/v2/events.cy.ts:10
- Unused import validate_401_Status.
import {
validate_200_Status,
validate_401_Status,
validate_expected_status,
getAPIBaseURL,
getTokenForV2,
} from '../../support/commands';
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
lukaszgryglicki
had a problem deploying
to
dev
GitHub Actions
Failure
lukaszgryglicki
had a problem deploying
to
dev
GitHub Actions
Failure
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
lukaszgryglicki
had a problem deploying
to
dev
GitHub Actions
Failure
lukaszgryglicki
had a problem deploying
to
dev
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/functional/cypress/e2e/v2/events.cy.ts (1)
33-57: Confirmvalidate_200_Statusis compatible with null response bodiesHere you both call
validate_200_Status(response)and assertresponse.bodyisnull. Ifvalidate_200_Statusstill enforces a non‑null body (as it did in earlier iterations), these assertions are contradictory and will make the test fail.Please confirm the helper’s implementation for V2:
- If it assumes a non‑null body, drop it here and assert only on
status+body === null, e.g.:- validate_200_Status(response); - // V2 API returns null body for successful POST requests - expect(response.body).to.be.null; + // V2 API returns 200 with null body for successful POST requests + expect(response.status).to.eq(200); + expect(response.body).to.be.null;
- If
validate_200_Statuswas updated to only check status and not body, this is fine as‑is; just ensure that’s explicitly true for V2 paths.
🧹 Nitpick comments (4)
tests/functional/cypress/e2e/v1/company.cy.ts (1)
11-11: TypeScript strict null check issue.Assigning
nullto astringtype will fail under strict null checks. Use a union type or initialize with an empty string.- let bearerToken: string = null; + let bearerToken: string | null = null;tests/functional/cypress/e2e/v1/signature.cy.ts (1)
11-11: TypeScript strict null check issue.Same as in
company.cy.ts, assigningnullto astringtype will fail under strict null checks.- let bearerToken: string = null; + let bearerToken: string | null = null;tests/functional/cypress/e2e/v1/repository.cy.ts (1)
11-11: TypeScript strict null check issue.Same pattern as other files. Consider using a union type.
- let bearerToken: string = null; + let bearerToken: string | null = null;tests/functional/cypress/e2e/v2/events.cy.ts (1)
7-21: Clarify ALLOW_FAIL env handling andbearerTokentype
Cypress.env('ALLOW_FAIL')is often a string when passed via CLI;=== 1will not match'1', so the override may never take effect unless configured as a numeric env in Cypress config.bearerTokenis typed asstringbut initialized withnull, which is invalid under strict null checks.Consider tightening both:
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + const allowFailEnv = Cypress.env('ALLOW_FAIL'); + const allowFail: boolean = !(Number(allowFailEnv) === 1); @@ - let bearerToken: string = null; + let bearerToken: string | null = null;This keeps behavior the same while being robust to
'1'as a string and TS-null‑safe.
📜 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 (9)
tests/functional/cypress/e2e/v1/company.cy.ts(1 hunks)tests/functional/cypress/e2e/v1/events.cy.ts(1 hunks)tests/functional/cypress/e2e/v1/gerrit.cy.ts(1 hunks)tests/functional/cypress/e2e/v1/project.cy.ts(1 hunks)tests/functional/cypress/e2e/v1/repository.cy.ts(1 hunks)tests/functional/cypress/e2e/v1/signature.cy.ts(1 hunks)tests/functional/cypress/e2e/v1/user.cy.ts(1 hunks)tests/functional/cypress/e2e/v2/events.cy.ts(1 hunks)tests/functional/cypress/e2e/v2/repository.cy.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/functional/cypress/e2e/v1/project.cy.ts
- tests/functional/cypress/e2e/v1/gerrit.cy.ts
- tests/functional/cypress/e2e/v1/events.cy.ts
- tests/functional/cypress/e2e/v1/user.cy.ts
🧰 Additional context used
🧬 Code graph analysis (2)
tests/functional/cypress/e2e/v1/signature.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(296-323)bearerToken(358-358)envToken(361-361)getTokenForV2(239-265)validate_200_Status(47-52)validate_expected_status(155-230)
tests/functional/cypress/e2e/v1/company.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(296-323)bearerToken(358-358)envToken(361-361)getTokenForV2(239-265)validate_200_Status(47-52)validate_expected_status(155-230)
⏰ 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 (9)
tests/functional/cypress/e2e/v1/company.cy.ts (1)
31-47: All tests in this file are skipped.All positive and negative test cases use
it.skip, meaning no tests will actually run. If this is intentional scaffolding for future enablement, consider adding a TODO comment at the top of the file to track when these should be enabled.tests/functional/cypress/e2e/v1/signature.cy.ts (2)
50-79: POST test creates persistent data without cleanup.This test creates a signature record using placeholder UUIDs. If successful, it leaves test data in the system. Consider:
- Adding cleanup in an
after()hook to delete created signatures- Using a beforeEach/afterEach pattern to manage test data lifecycle
- Verifying the endpoint will reject invalid project/user IDs before enabling
If the V1 API rejects placeholder UUIDs, this test will fail at
validate_200_Status.
85-128: LGTM!The 401 authentication test structure is well-organized, using parameterized test cases with clear titles and appropriate assertions for unauthorized responses.
tests/functional/cypress/e2e/v1/repository.cy.ts (1)
95-101: Good documentation of API quirk.The test correctly documents that the V1 API returns 200 with an error object in the body for invalid UUIDs, rather than the more typical 400 response. This behavior documentation is valuable for understanding API semantics.
tests/functional/cypress/e2e/v2/repository.cy.ts (2)
75-89: LGTM!The test appropriately handles the dual expected outcomes (200 or 400) for the corporate signature callback, acknowledging that parameter validation behavior may vary.
20-73: Skip reasons are well documented.The skipped tests have clear explanations for why they cannot run (GitHub integration issues, XML/DocuSign callback format mismatches). This documentation helps future maintainers understand when these tests can be enabled.
tests/functional/cypress/e2e/v2/events.cy.ts (3)
59-76: Positive /clear-cache test is well‑structuredThe happy‑path test for
POST /clear-cachecorrectly asserts 200, that the body is an object, and that it contains{ status: 'OK' }, matching the documented V2 behavior. No issues spotted here.
82-122: 401 “no token” coverage looks solidThe table‑driven loop over authenticated endpoints without a token, asserting 401,
statusText === 'Unauthorized', and a string body containing “authorization” provides clear and reusable negative coverage. This is a good pattern and matches the intended behavior.
124-187: 4xx validation/error cases are comprehensively coveredThe
casesarray plusvalidate_expected_statuscall gives concise coverage for missing parameters and method‑not‑allowed flows (400 and 405), including explicit comments about auth vs method precedence. This looks consistent and maintainable; no changes needed from my side.
Cypress E2E test coverage for V2 and V1 APIs.
Signed-off-by: Lukasz Gryglicki [email protected]
Assisted by OpenAI
Assisted by GitHub Copilot