End to end test for dataset overview and issue flow#694
Conversation
WalkthroughThe changes in this pull request involve modifications to the HTML template for the organisation overview page, enhancing its semantic structure by adjusting heading levels and adding Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Coverage Report
File CoverageNo changed files found. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
test/PageObjectModels/landingPage.js (1)
4-7: Consider making the base path configurableWhilst the current implementation works, consider making the base path configurable through environment variables or a configuration file. This would make the test suite more flexible across different environments (e.g., local development, staging, production).
Example implementation:
export default class LandingPage extends BasePage { constructor (page) { - super(page, '/') + super(page, process.env.BASE_PATH || '/') } }test/PageObjectModels/organisationOverviewPage.js (1)
5-7: Add JSDoc documentation and consider input validationConsider adding JSDoc documentation for the constructor parameters and validating the
organisationIdbefore using it in the URL path.+/** + * @param {import('playwright').Page} page - Playwright page instance + * @param {Object} options - Configuration options + * @param {string} options.organisationId - The organisation identifier + */ constructor (page, options) { + if (!options?.organisationId) { + throw new Error('organisationId is required') + } super(page, `/organisations/${options.organisationId}`) }test/PageObjectModels/organisationsPage.js (1)
4-7: Consider making the URL path configurableThe hardcoded URL path '/organisations' could be problematic if the base URL structure changes. Consider moving this to a configuration file or environment variable.
+import { ROUTES } from '../config/routes' export default class OrganisationsPage extends BasePage { constructor (page) { - super(page, '/organisations') + super(page, ROUTES.ORGANISATIONS) }test/PageObjectModels/BasePage.js (1)
61-62: Consider adding JSDoc documentationTo improve maintainability, consider adding JSDoc documentation to describe the method parameters and their expected structure:
+ /** + * Instantiates and verifies a new page object + * @param {typeof BasePage} PageType - The page class to instantiate + * @param {boolean} [skipVerification=false] - Whether to skip page verification + * @param {Object} [pageOptions={}] - Configuration options for the page + * @returns {Promise<BasePage>} The instantiated page object + */ async verifyAndReturnPage (PageType, skipVerification = false, pageOptions = {}) {test/acceptance/dataset_overview.test.js (2)
6-26: Consider extracting test data to fixturesThe organisation details ("Lambeth", "London Borough of Lambeth", "local-authority:LBH") are hard-coded. Consider moving these to a test fixtures file for better maintainability.
Example structure:
// test/fixtures/organisations.js export const LAMBETH = { searchTerm: 'Lambeth', name: 'London Borough of Lambeth', id: 'local-authority:LBH' }Consider splitting multiple assertions into separate test cases
The test verifies multiple aspects (navigation, mandatory datasets, ODP datasets). Consider splitting these into separate test cases for better isolation and maintenance.
Example structure:
test('displays correct page title after navigation', async ({page}) => { // Setup and navigation expect(await page.locator('h1').innerText()) .toEqual('London Borough of Lambeth overview') }) test('displays mandatory datasets section correctly', async ({page}) => { // Setup and navigation expect(await page.locator('[data-testid=datasetsMandatory] h2').innerText()) .toEqual('Datasets London Borough of Lambeth must provide') expect(await page.locator('[data-testid=datasetsMandatory] .govuk-task-list li').count()) .toEqual(1) })
5-45: Consider adding error scenario testsThe test suite covers the happy path but lacks tests for error scenarios. Consider adding tests for:
- Organisation not found
- Invalid dataset IDs
- Network errors
- Permission-related issues
This would improve the robustness of the test suite and help catch potential issues early.
src/views/organisations/overview.html (1)
136-148: Consider enhancing accessibility of membership information.The implementation is good, but consider wrapping the membership information in an ARIA live region to improve screen reader experience.
- <p class="org-membership-info">{{ organisation.name}} is a member of the Open Digital Planning programme.</p> + <p class="org-membership-info" role="status" aria-live="polite">{{ organisation.name}} is a member of the Open Digital Planning programme.</p> - <p class="org-membership-info">{{ organisation.name}} is not a member of the Open Digital Planning programme.</p> + <p class="org-membership-info" role="status" aria-live="polite">{{ organisation.name}} is not a member of the Open Digital Planning programme.</p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/views/organisations/overview.html(3 hunks)test/PageObjectModels/BasePage.js(1 hunks)test/PageObjectModels/datasetOverviewPage.js(1 hunks)test/PageObjectModels/landingPage.js(1 hunks)test/PageObjectModels/organisationOverviewPage.js(1 hunks)test/PageObjectModels/organisationsPage.js(1 hunks)test/acceptance/dataset_overview.test.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
test/acceptance/dataset_overview.test.js (1)
Learnt from: GeorgeGoodall-GovUk
PR: digital-land/submit#510
File: test/unit/views/organisations/dataset-overview.test.js:3-3
Timestamp: 2024-11-12T10:13:49.419Z
Learning: Ensure to verify import paths in all relevant test files before flagging inconsistencies.
🔇 Additional comments (9)
test/PageObjectModels/datasetOverviewPage.js (2)
1-1: LGTM! Clean and focused import statement.
The import is properly structured and follows the module organisation pattern.
1-7: Verify test coverage for the page object.
Given the PR objective of maintaining 80% code coverage, we should verify the test implementation for this page object.
✅ Verification successful
Let me check the actual implementation and test coverage by examining the related files more closely.
Let me check the actual test implementation for the dataset overview page to verify coverage.
Test coverage appears to be sufficient
The DatasetOverviewPage is adequately tested through:
- Acceptance tests in
dataset_overview.test.jsthat verify the page navigation and content - Comprehensive unit tests in
dataset-overview.test.jscovering the page's rendering and functionality - Integration with OrganisationOverviewPage where it's properly instantiated and verified
The test implementation meets the 80% coverage requirement through:
- End-to-end navigation testing
- Component rendering verification
- Integration testing with parent pages
- Extensive unit test coverage of the view template
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test file existence and coverage
# Look for corresponding test file
fd "datasetOverviewPage.test.js" test/
# Check for usage in acceptance tests
rg -l "DatasetOverviewPage" test/ --type js
Length of output: 177
Script:
#!/bin/bash
# Check implementation details and test usage
rg -A 5 "DatasetOverviewPage" test/PageObjectModels/organisationOverviewPage.js
# Check if there are any test files that might be using this page object
rg "DatasetOverviewPage" test/ --type js -C 5
# Look for any acceptance tests in the e2e or integration directories
fd ".*\.test\.js|.*\.spec\.js" test/e2e test/integration
# Check if there are any dataset-related test files
fd "dataset.*\.(test|spec)\.js" test/
Length of output: 3340
Script:
#!/bin/bash
# Check the dataset overview acceptance test implementation
cat test/acceptance/dataset_overview.test.js
# Check the dataset overview unit test implementation
cat "test/unit/views/organisations/dataset-overview.test.js"
# Check if there are any page object assertions in the integration tests
rg -A 5 "DatasetOverviewPage" test/integration/
Length of output: 10727
test/PageObjectModels/landingPage.js (2)
1-2: Imports look good!
The imports are clean, focused, and logically ordered.
9-12: 🛠️ Refactor suggestion
Enhance test reliability with better selectors and error handling
The current implementation has several potential reliability issues:
- Using text selectors is fragile as they're sensitive to content changes
- No explicit wait or visibility check before clicking
- No error handling for failed interactions
Consider implementing these improvements:
async clickStartNow () {
- await this.page.click('text=Submit and update your data now')
+ try {
+ await this.page.waitForSelector('[data-testid="start-now-button"]', { state: 'visible' })
+ await this.page.click('[data-testid="start-now-button"]')
+ } catch (error) {
+ throw new Error(`Failed to click start now button: ${error.message}`)
+ }
return await super.verifyAndReturnPage(OrganisationsPage)
}Let's verify if the corresponding HTML template includes the data-testid attribute:
✅ Verification successful
Consider using existing button attributes for more reliable test selectors
The button in the template already has distinctive attributes that can be used for more reliable test selectors:
href="/organisations"class="govuk-button govuk-button--inverse govuk-!-margin-top-6 govuk-!-margin-bottom-0 govuk-button--start"
Recommend updating the test to use these existing attributes instead:
async clickStartNow () {
- await this.page.click('text=Submit and update your data now')
+ try {
+ const buttonSelector = 'a.govuk-button[href="/organisations"]'
+ await this.page.waitForSelector(buttonSelector, { state: 'visible' })
+ await this.page.click(buttonSelector)
+ } catch (error) {
+ throw new Error(`Failed to click start now button: ${error.message}`)
+ }
return await super.verifyAndReturnPage(OrganisationsPage)
}This approach:
- Uses existing stable attributes instead of adding new ones
- Maintains error handling and visibility checks
- Follows GOV.UK Frontend conventions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the landing page template includes the suggested data-testid
# Search for the button in template files
rg -l 'Submit and update your data now' --type html
Length of output: 76
Script:
#!/bin/bash
# Check the content of the landing page template to see if data-testid is used
rg -A 5 -B 5 'Submit and update your data now' src/views/landing.html
Length of output: 644
test/PageObjectModels/organisationOverviewPage.js (2)
1-4: LGTM! Proper implementation of Page Object Model pattern
The class structure and imports are well-organised, following the Page Object Model design pattern effectively.
9-15: 🛠️ Refactor suggestion
Enhance method robustness and documentation
The method needs documentation, error handling, and could be simplified by using the organisationId from constructor.
+/**
+ * Navigates to the dataset overview page
+ * @param {string} organisationId - The organisation identifier
+ * @param {string} datasetId - The dataset identifier
+ * @returns {Promise<DatasetOverviewPage>} The dataset overview page instance
+ * @throws {Error} If the navigation fails
+ */
-async clickOnDataset (organisationId, datasetId) {
+async clickOnDataset (datasetId) {
+ try {
+ const organisationId = this.options.organisationId
await this.page.click(`a[href="/organisations/${organisationId}/${datasetId}/overview"]`)
return await super.verifyAndReturnPage(DatasetOverviewPage, false, {
organisationId, datasetId
})
+ } catch (error) {
+ throw new Error(`Failed to navigate to dataset overview: ${error.message}`)
+ }
}Additionally, consider adding data-testid attributes to make the selectors more reliable:
test/PageObjectModels/organisationsPage.js (1)
1-21: Verify test coverage for this page object
Let's ensure this page object is properly tested and integrated with the test suite.
✅ Verification successful
Page object is well integrated and properly tested
The OrganisationsPage is adequately tested and integrated:
- Used in acceptance test
dataset_overview.test.jswith full flow coverage (navigation, search, and selection) - Integrated with LandingPage for proper page flow transitions
- Has corresponding middleware tests in
organisations.middleware.test.jsthat verify the underlying functionality - Implementation aligns with the test coverage and usage patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find test files that use OrganisationsPage
echo "Files using OrganisationsPage:"
rg -l "OrganisationsPage" test/
# Check for existing test cases
echo "\nTest cases for organisations page:"
rg -A 5 "describe.*['\"]Organisations" test/
Length of output: 314
Script:
#!/bin/bash
# Let's check how OrganisationsPage is used in landingPage.js
echo "Usage in landingPage.js:"
rg -A 10 "OrganisationsPage" test/PageObjectModels/landingPage.js
# Check for any test files that might be using these page objects
echo "\nTest files in the suite:"
fd ".*\.test\.js|.*\.spec\.js" test/
# Check for any describe blocks mentioning 'organisation' (alternative spelling)
echo "\nTest cases with 'organisation':"
rg -i "describe.*['\"].*organisation.*['\"]" test/
Length of output: 3818
Script:
#!/bin/bash
# Let's check the integration tests that might be using these page objects
echo "Checking integration tests for organisation-related flows:"
rg -A 10 "new LandingPage|new OrganisationsPage" test/integration/
# Let's examine the organisations middleware test
echo "\nOrganisations middleware test content:"
cat test/unit/middleware/organisations.middleware.test.js
# Check for any acceptance tests that might use these pages
echo "\nChecking acceptance tests for organisation flows:"
rg -A 10 "new LandingPage|new OrganisationsPage" test/acceptance/
Length of output: 4414
test/PageObjectModels/BasePage.js (1)
61-62: Well-structured enhancement to page instantiation
The addition of the pageOptions parameter with a default empty object maintains backwards compatibility whilst providing flexibility for configuring page objects. This change effectively supports the implementation of end-to-end tests for various page types.
src/views/organisations/overview.html (1)
125-132: Well-structured implementation of testable mandatory datasets section.
The addition of data-testid attribute enhances testability whilst maintaining semantic HTML structure. This aligns well with the PR's e2e testing objectives.
DilwoarH
force-pushed
the
612-dataset-overview-test
branch
from
dc78a1d to
c5b3920
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
test/PageObjectModels/datasetIssuesPage.js (1)
1-7: Implementation follows good practices but needs enhancementThe Page Object Model implementation follows good practices by extending BasePage and properly handling URL construction. However, given that this page represents the dataset issues view, it should include methods for interacting with issue-related elements.
Consider adding these essential methods to support the end-to-end testing requirements:
export default class DatasetIssuesPage extends BasePage { constructor (page, options) { super(page, `/organisations/${options.organisationId}/${options.datasetId}`) } + + async getIssuesList() { + return this.page.$$('[data-testid="issue-item"]') + } + + async getIssueDetails(issueId) { + return this.page.$(`[data-testid="issue-${issueId}"]`) + } + + async isIssueTableVisible() { + return this.page.isVisible('[data-testid="issues-table"]') + } }test/PageObjectModels/datasetTablePage.js (1)
1-7: Add JSDoc documentationConsider adding JSDoc documentation to improve code maintainability:
import BasePage from './BasePage' +/** + * Page Object Model for the dataset table view. + * @extends BasePage + */ export default class DatasetTablePage extends BasePage { + /** + * Creates an instance of DatasetTablePage. + * @param {import('playwright').Page} page - The Playwright page object + * @param {Object} options - The options object + * @param {string} options.organisationId - The organisation ID + * @param {string} options.datasetId - The dataset ID + */ constructor (page, options) { super(page, `/organisations/${options.organisationId}/${options.datasetId}/data`) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
src/views/organisations/overview.html(3 hunks)test/PageObjectModels/BasePage.js(1 hunks)test/PageObjectModels/datasetIssuesPage.js(1 hunks)test/PageObjectModels/datasetOverviewPage.js(1 hunks)test/PageObjectModels/datasetTablePage.js(1 hunks)test/PageObjectModels/landingPage.js(1 hunks)test/PageObjectModels/organisationOverviewPage.js(1 hunks)test/PageObjectModels/organisationsPage.js(1 hunks)test/acceptance/dataset_overview.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/views/organisations/overview.html
- test/PageObjectModels/BasePage.js
- test/PageObjectModels/datasetOverviewPage.js
- test/PageObjectModels/landingPage.js
- test/PageObjectModels/organisationOverviewPage.js
- test/PageObjectModels/organisationsPage.js
- test/acceptance/dataset_overview.test.js
🔇 Additional comments (1)
test/PageObjectModels/datasetIssuesPage.js (1)
5-5: Verify URL pattern consistency
The URL pattern should be verified against the application's routing configuration to ensure it matches the expected format.
✅ Verification successful
URL pattern is consistent with application routing
Based on the search results, the URL pattern /organisations/${organisationId}/${datasetId} is consistently used throughout the codebase:
- The pattern appears in multiple page objects (datasetIssuesPage.js, datasetTablePage.js, datasetOverviewPage.js)
- It's used in route definitions and middleware (issueDetails.middleware.js, dataview.middleware.js)
- Test files confirm this URL structure is expected (various test files under test/unit/)
- The base route '/organisations' is properly registered in the application's routing setup (src/serverSetup/routes.js)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check URL pattern consistency across the codebase
# Expected: Find routes or constants defining the URL pattern for dataset issues
# Search for route definitions
rg -g '*.{js,jsx,ts,tsx}' -A 5 "organisations/:.*/:.*"
# Search for similar URL constructions
ast-grep --pattern 'organisations/${$_}/${$_}'
Length of output: 105
Script:
#!/bin/bash
# Let's try different approaches to find URL patterns
# Search for routes or paths containing 'organisations'
rg -g '*.{js,jsx,ts,tsx}' '/organisations'
# Search for route definitions using different patterns
rg -g '*.{js,jsx,ts,tsx}' 'path.*organisations'
# Look for similar URL construction in page objects
fd datasetIssuesPage.js --exec cat {}
# Search for router configuration files
fd -g '*router*.{js,jsx,ts,tsx}' --exec cat {}
Length of output: 5828
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
3 participants
What type of PR is this? (check all applicable)
Description
End to end test for dataset overview and issue flow
Related Tickets & Documents
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
QA sign off
Summary by CodeRabbit
New Features
Tests