37 test sprint 1 code #45
Conversation
… 37-test-sprint-1-code
Make tests for testable interactions in the application for code from sprint 1
WalkthroughAdds Cypress end-to-end tests and test tooling: two new test suites (shaded lighting, measurement tool), updates and renames in existing Cypress tests, a new Cypress config flag, a npm "test" script, .gitignore update, and README instructions for running Cypress. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / CI
participant Cypress as Cypress Runner
participant Browser as Chrome
participant App as Dev Server (http://localhost:5173)
note over Cypress,Browser #DDEBF7: Test orchestration
Dev->>Cypress: npm run test (cypress run --browser chrome)
Cypress->>Browser: launch & load test spec
Browser->>App: GET / (navigate to http://localhost:5173/#)
App->>Browser: Render UI (Potree viewer, controls)
Browser->>App: UI interactions (clicks, toggles, input)
App->>Browser: DOM updates / visual changes
Browser->>Cypress: assertions (visible, count, state)
Cypress->>Dev: exit code / artifacts (screenshots on failure)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
Comment |
![coderabbit[bot]](https://avatars.git.ntnu.no/u/70329?s=60){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
cypress/e2e/filtering.cy.js (2)
1-7: Consider adding assertions to verify filtering behavior.The test only clicks the filtering button without verifying the expected outcome. Consider adding assertions to check that filtering was actually applied or that the UI state changed as expected.
Example:
describe('Accepted filtering', () => { it('test accepted filter button', function () { cy.visit('http://localhost:5173/#') + cy.get('#doAcceptedFiltering').should('exist') cy.get('#doAcceptedFiltering').click() + // Add assertion to verify filtering was applied + cy.get('#doAcceptedFiltering').should('have.class', 'active') }) })
9-24: Add assertions to verify elevation control changes.The test clicks various elevation controls but doesn't verify that the elevation settings actually changed. Consider adding assertions to check the applied elevation values or visible changes.
cypress/e2e/shadedLighting.cy.js (3)
1-25: Optimize withbeforeEachand configurebaseUrl.The URL
http://localhost:5173/#is repeated in every test. Consider:
- Adding a
baseUrltocypress.config.js- Using a
beforeEachhook to avoid repetitionApply this pattern:
describe('Shaded Lighting', () => { + beforeEach(() => { + cy.visit('/') + }) + it('Toggle the EDL checkbox on and off', () => { - cy.visit('http://localhost:5173/#') cy.get('#chkEDLEnabled').uncheck() cy.get('#chkEDLEnabled').check() }) it('Change radius', function () { - cy.visit('http://localhost:5173/#') cy.get('div.potree_container').click() cy.get('#sldEDLRadius span.ui-corner-all').click() }) // ... remaining tests })Then configure
baseUrlincypress.config.js:{ e2e: { baseUrl: 'http://localhost:5173' } }
1-25: Add assertions to verify EDL setting changes.The tests interact with EDL controls but don't verify that the settings actually changed. Consider adding assertions to check:
- Checkbox state after toggle
- Slider values after adjustment
- Visual changes if applicable
Example:
it('Toggle the EDL checkbox on and off', () => { cy.visit('http://localhost:5173/#') - cy.get('#chkEDLEnabled').uncheck() - cy.get('#chkEDLEnabled').check() + cy.get('#chkEDLEnabled').uncheck().should('not.be.checked') + cy.get('#chkEDLEnabled').check().should('be.checked') })
8-12: Container click may be fragile - consider adding context.Clicking
div.potree_containerwithout a specific coordinate or context might produce inconsistent results depending on what's rendered in the container. Consider adding a comment explaining why this click is necessary, or use a more specific interaction if possible.cypress/e2e/globeRendering.cy.js (1)
15-21: Consider adding assertions for interaction verification.The test verifies that the viewer and container exist but doesn't actually test any interactions with the globe. Consider adding user interactions (e.g., camera movements, clicks) and assertions to verify they work correctly.
Example:
it('Render the globe and test interactions with the globe', () => { cy.visit('http://localhost:5173/#') cy.wait(6000) cy.window({ timeout: 20000 }).its('cesiumViewer').should('exist') cy.get('#cesiumContainer').should('exist') + + // Test interaction: click on the globe + cy.get('#cesiumContainer canvas').click(400, 300) + // Verify camera position changed or other interaction effect })cypress/e2e/viewAttribute.cy.js (4)
9-9: Remove trailing spaces from test descriptions.Several test descriptions have trailing spaces that should be removed for consistency.
Apply this diff:
- it('Delete view attribute ', function () { + it('Delete view attribute', function () { - it('Copy attribute positions ', function () { + it('Copy attribute positions', function () { - it('Switch between attributes ', function () { + it('Switch between attributes', function () { - it('Open and close List of Measurements ', function () { + it('Open and close List of Measurements', function () {Also applies to: 20-20, 27-27, 40-40
1-50: Optimize withbeforeEachand configurebaseUrl.Similar to the shadedLighting tests, the URL is repeated in every test. Use a
beforeEachhook and configurebaseUrlincypress.config.jsto reduce duplication.Apply this pattern:
describe('View Attribute', () => { + beforeEach(() => { + cy.visit('/') + }) + it('Places the view attribute', () => { - cy.visit('http://localhost:5173/#') cy.get('#tools img[data-i18n="[title]tt.point_measurement"]').click() cy.get('#potree_render_area canvas[tabindex="2222"]').click() }) // ... remaining tests })
1-50: Add assertions to verify attribute operations.The tests interact with view attributes but don't verify the expected outcomes. Consider adding assertions to check:
- Attribute was created after placement
- Attribute was removed after deletion
- Attribute was copied successfully
- Correct attribute is selected when switching
- Measurements list visibility state
Example:
it('Places the view attribute', () => { cy.visit('http://localhost:5173/#') cy.get('#tools img[data-i18n="[title]tt.point_measurement"]').click() cy.get('#potree_render_area canvas[tabindex="2222"]').click() + // Verify attribute was created + cy.get('#scene_object_properties').should('be.visible') + cy.get('#g_point_body').children().should('have.length.at.least', 1) })
27-38: Complex test flow could benefit from comments.The "Switch between attributes" test creates three points and then clicks them in a specific sequence. Adding comments explaining the expected behavior would improve test maintainability.
Example:
it('Switch between attributes ', function () { cy.visit('http://localhost:5173/#') + // Create three point measurements cy.get('#tools img[data-i18n="[title]tt.point_measurement"]').click() cy.get('#potree_render_area canvas[tabindex="2222"]').click() cy.get('#tools img[data-i18n="[title]tt.point_measurement"]').click() cy.get('#potree_render_area canvas[tabindex="2222"]').click() cy.get('#tools img[data-i18n="[title]tt.point_measurement"]').click() cy.get('#potree_render_area canvas[tabindex="2222"]').click() + // Switch between the created points cy.get('#g_point_body span[title="Point #2"]').click() cy.get('#g_point_body div:nth-child(1)').click() cy.get('#g_point_body span[title="Point #3"]').click() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)cypress/e2e/filtering.cy.js(1 hunks)cypress/e2e/globeRendering.cy.js(2 hunks)cypress/e2e/shadedLighting.cy.js(1 hunks)cypress/e2e/viewAttribute.cy.js(1 hunks)
🔇 Additional comments (3)
README.md (1)
50-65: Excellent documentation for Cypress testing.The new Cypress Testing section clearly documents how to run the development server and execute tests. The instructions are concise and properly formatted.
cypress/e2e/globeRendering.cy.js (1)
2-2: Test description updates improve clarity.The renamed test descriptions better reflect what each test validates. "Check that the viewer is rendered correctly" and "Render the globe and test interactions with the globe" are more precise than the previous versions.
Also applies to: 15-15
cypress/e2e/filtering.cy.js (1)
5-5: Selector update verified as correct.The change to
#doAcceptedFilteringis confirmed. The element with this ID is created insrc/AcceptedFiltering/threePanels.js(line 266) and the test selector now correctly matches the application's button ID.
There was a problem hiding this comment.
Good work! Have some comments.
Can't make a suggestion for the .gitignore as it is not changed, but we should add /cypress/screenshots/ to the .gitignore so that test screenshots are ignored by git.
I also got this error: SharedImageManager::ProduceSkia: Trying to Produce a Skia representation from a non-existent mailbox.. Was fixed by running the tests in the chrome browser. We should make a script in package.json so we can just write npm run test instead of npc cypress run. Under scripts, just add "test": "cypress run --browser chrome".
…ecification under scripts
…ackage.json and update readme
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cypress/e2e/shadedLighting.cy.js (1)
1-25: Extract hardcoded URL to configuration.The URL
http://localhost:5173/#is duplicated across all four tests. Consider using Cypress'sbaseUrlin yourcypress.config.jsor extracting it to a constant to improve maintainability.Example using
baseUrlincypress.config.js:export default { e2e: { baseUrl: 'http://localhost:5173' } }Then update tests to use:
- cy.visit('http://localhost:5173/#') + cy.visit('/#')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
.gitignore(1 hunks)cypress/e2e/shadedLighting.cy.js(1 hunks)cypress/e2e/viewAttribute.cy.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/e2e/viewAttribute.cy.js
🔇 Additional comments (1)
.gitignore (1)
29-30: LGTM!Adding Cypress screenshots to
.gitignoreis standard practice for excluding test artifacts from version control.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
cypress/e2e/measurementTool.cy.js (1)
6-6: Consider more reliable selectors for the canvas element.The selector
canvas[tabindex="2222"]is unusual and fragile. If the tabindex changes, all tests will break. Consider adding adata-testidattribute to the canvas for more stable test selection.In your application code:
<canvas data-testid="measurement-canvas" tabindex="2222">Then in tests:
-cy.get('#potree_render_area canvas[tabindex="2222"]').click() +cy.get('[data-testid="measurement-canvas"]').click()Also applies to: 17-17, 20-20, 27-27, 35-35, 37-37, 39-39, 50-50, 52-52
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cypress/e2e/measurementTool.cy.js(1 hunks)
🔇 Additional comments (1)
cypress/e2e/measurementTool.cy.js (1)
2-11: Good test structure with proper assertions.This test correctly verifies both the visibility of the properties panel and the creation of measurement elements.
|
Added the specification now |
Issue number
Closes 37
Description
Made tests for interactions on the applications made under sprint 1.
Testing steps
See readme for a guide on how to run tests
Screenshots (optional)
Summary by CodeRabbit
Documentation
Tests
Chores