Merged
Conversation
- Update all error mock functions in mockRoutes.ts to call page.unroute() before adding new error handlers, ensuring error routes take precedence over default success handlers registered by setupDefaultMocks() - This fixes the issue where Playwright evaluates routes in FIFO order, causing success handlers to always win over error handlers - Update SKIPPED_TESTS_ROADMAP.md to reflect actual Playwright route helper implementation instead of the originally proposed MSW approach - Add docs/phase1-follow-up.md to document resolved issues and Phase 2 readiness checklist Affected functions: - setupNetworkErrorMock - setupServerErrorMock - setupInvalidCredentialsMock - setupGeocodingErrorMock - setupAlertCreationErrorMock
🐳 Docker Build SuccessfulYour PR has been built and tested successfully! Test Results: ✅ All tests passed Docker Hub PublishingTo enable PR image publishing:
You can still test locally by building the Dockerfile: docker build -t pr-14 .
docker run -p 8080:80 pr-14 |
Add test-friendly data attributes for reliable E2E test automation:
Map Container (MapContainer.tsx):
- Add data-testid="map-container" for reliable selector
- Add data-map-loaded attribute that tracks map load state
Selection Marker (useMapInteraction.ts):
- Add data-testid="selection-marker" to user-selected location marker
- Add data-coordinates attribute with lng,lat values
Alert Markers (useMapAlerts.ts):
- Add data-testid="alert-marker-{id}" to each alert marker
- Add data-alert-name and data-coordinates attributes
Test Helpers (MapPage.ts):
- Update selectors to use data-testid attributes instead of fragile CSS classes
- Add waitForMapLoad() with data-map-loaded detection
- Add new marker helper methods: waitForAlertMarker, getMarkerCoordinates,
clickAlertMarker, getAlertMarkerCount
- Improve assertions with expectAlertMarkersCount
Enable skipped tests (10 tests):
- Alert Creation Flow (4 tests)
- Map Interactions (2 tests)
- Error Handling (2 tests)
- Logout (2 tests)
All tests now skip in local environment and run in CI (CI=true)
Collaborator
Author
|
@codex review |
- Update executive summary to reflect 100% test coverage - Mark Phase 2 as completed with implementation details - Update test inventory to show all 21 tests are now active - Update document metadata (version 2.0, status complete) Phase 2 implementation added: - data-testid attributes for map container and markers - Reliable MapPage test helper methods - CI skip condition for all map tests
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The app uses component state switching on "/" route instead of separate URL routing for the project selection step. Replace the failing URL assertion `expect(page).toHaveURL(/projects|select-projects/)` with a DOM assertion checking for the "Back to Map" button which is unique to the project selection UI.
Add setupDefaultMocks() calls to both Logout tests to ensure API mocks are in place before loginWithValidCredentials() is called. Without this, login requests would hit the real backend and fail.
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
test.skip() expects a boolean as the first argument, not a function. Passing a function makes the condition always truthy, causing tests to be skipped everywhere including CI.
Collaborator
Author
|
@codex review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The previous selector counted all mapboxgl/maplibregl markers including alert markers, causing the test to fail. Now uses the specific data-testid="selection-marker" selector to count only selection markers.
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Add ProjectSelectionPage and AlertFormPage page objects to complete the alert creation flow in E2E tests. - ProjectSelectionPage: handles project checkbox selection, continue to alert form, back to map navigation - AlertFormPage: handles form inputs (times, sourceId, alertName), form submission, success/error state validation - Update create-alert test to use new page objects for complete end-to-end alert creation flow This improves test maintainability and provides reusable abstractions for the project selection and alert form UI interactions.
- Mark Phase 3 (Additional Page Objects) as complete - Update executive summary to reflect Phase 3 implementation - Add documentation for created files and updated tests
Collaborator
Author
|
@codex review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
toHaveURL('/') matches the complete URL string, not the path.
Use /\/$/ regex to match URLs ending with '/' so the assertion
works with full URLs like http://localhost:4173/
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Add data-testid attributes to key UI elements for improved test
reliability and resilience to UI changes.
ProjectSelection.tsx:
- data-testid="project-selection" (container)
- data-testid="project-row-{id}" and data-project-name (rows)
- data-testid="project-checkbox-{id}" (checkboxes)
- data-testid="selected-projects-summary/count"
- data-testid="continue-to-alert-button"
AlertForm.tsx:
- data-testid="alert-form" (container)
- data-testid="coordinates-display"
- data-testid="selected-projects-display"
- data-testid="alert-submit-button"
- data-testid="alert-error-message"
- data-testid="alert-validation-error"
Page objects updated to use new selectors instead of fragile
CSS class or text matchers.
- Mark Phase 4 (Component Test IDs) as complete - Update executive summary to reflect all phases complete - Add documentation for test IDs and modified files - Include test ID reference table
- Fix expectNoValidationError() to use toHaveCount(0) instead of toBeHidden() for conditionally rendered elements - Add data-selected-count attribute to continue button for reliable count access without parsing i18n text - Add data-testid="loading-indicator" for consistent test selectors - Update getSelectedProjectCount() to use data attribute instead of text parsing - Update documentation with new test IDs
AlertForm.tsx: - Add data-coordinates attribute for consistent coordinate access - Add data-testid="alert-retry-button" for retry button AlertFormPage.ts: - Add retryButton locator - Update getDisplayedCoordinates to use data-coordinates attribute - Add NaN validation for coordinate parsing - Add expectNoSubmissionError method for symmetry ProjectSelectionPage.ts: - Update continueToAlertForm to wait for container instead of input - Add NaN validation for getSelectedProjectCount - Fix expectProjectsLoaded to check if loading indicator exists before waiting - Add expectSelectedProjectsSummaryVisible method - Add expectNoSelectedProjectsSummary method for conditionally rendered element All page object methods now: - Use data-testid selectors consistently - Properly handle conditionally rendered elements with toHaveCount(0) - Include NaN validation for numeric parsing - Have symmetric assert/expectNot method pairs
Collaborator
Author
|
@codex is this PR ready for merge? |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
luandro
deleted the
claude/skipped-tests-roadmap-setup-01E5adRkqR17XrcYbjrEktcL
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.