Add unit and browser tests for network map layers, upgrade deck.gl to 9.3#380
Add unit and browser tests for network map layers, upgrade deck.gl to 9.3#380amarant wants to merge 12 commits into
Conversation
a02f594 to
89dacb4
Compare
89dacb4 to
795929f
Compare
8d18d04 to
a360f24
Compare
a360f24 to
0f8ea52
Compare
|
ba7fff3 to
67e26d9
Compare
- Implemented tests for ArrowLayer, ForkLineLayer, ParallelPathLayer, ScatterplotLayerExt, and LineLayer. - Added screenshot comparison functionality for visual regression testing. - Created utility functions for rendering layers and setting up test environments. - Introduced network and geo data creation functions for testing purposes. - Added browser setup entrypoint for package-level shared initialization. - Updated App.tsx to utilize large network and geo data for testing. - Added new test cases for arrow, fork, parallel path, and scatterplot layers to handle large network scenarios. - Introduced large network and geo data creation functions in network-fixtures.ts. - Adjusted browser rendering setup to accommodate larger viewport sizes. - Enhanced logging for current view state in network-map.tsx. - Updated Vitest configuration for increased viewport dimensions in tests. - Updated @deck.gl and @luma.gl dependencies to 9.3.0 versions in package.json and network-map-layers/package.json. Signed-off-by: Arnaud Marant <arnaud.marant@rte-france.com>
Signed-off-by: Arnaud Marant <arnaud.marant@rte-france.com>
add image approximate comparison in tests sonar ignore test fixtures Signed-off-by: Arnaud Marant <arnaud.marant@rte-france.com>
Signed-off-by: Arnaud Marant <arnaud.marant@rte-france.com>
Signed-off-by: Arnaud Marant <arnaud.marant@rte-france.com>
Signed-off-by: Arnaud Marant <arnaud.marant@rte-france.com>
…right browser installation step Signed-off-by: Arnaud Marant <arnaud.marant@rte-france.com>
67e26d9 to
3a4a14c
Compare
sBouzols
left a comment
There was a problem hiding this comment.
Tests OK in GridSuite app
Some minor comments
…ility files Signed-off-by: Arnaud Marant <arnaud.marant@rte-france.com>
… and add missing configDefaults import Signed-off-by: Arnaud Marant <arnaud.marant@rte-france.com>
NathanDissoubray
left a comment
There was a problem hiding this comment.
You have 10 tests that lack assertions, as pointed out by Sonar, is there a reason for this ?
| "@luma.gl/core": "~9.2.0", | ||
| "@luma.gl/engine": "~9.2.0", | ||
| "@luma.gl/shadertools": "~9.2.0" | ||
| "@deck.gl/core": "~9.3.0", |
There was a problem hiding this comment.
Could you tell me if there is a reason why the versions are different (9.3.0 here and 9.3.4 below)?
There was a problem hiding this comment.
I synchronized the @powsybl/network-viewer package to ~9.3.0
@NathanDissoubray None of the tests lack assertions, it's just that sonar doesn't understand this is an assertion.
|
…n 9.3.0 Signed-off-by: Arnaud Marant <arnaud.marant@rte-france.com>
|
@amarant there are sonar issues regarding using chains I'm not sure what this means, but could you take a look at it ? |
…access to properties Signed-off-by: Arnaud Marant <arnaud.marant@rte-france.com>
| parserOptions: { | ||
| projectService: true, | ||
| projectService: { | ||
| allowDefaultProject: ['./setupBrowserTests.ts'], |
There was a problem hiding this comment.
Where is this file setupBrowserTests.ts ?
There was a problem hiding this comment.
it's a leftover of intermediate work, but browser tests didn't need a setup. I will remove it.
…rserOptions Signed-off-by: Arnaud Marant <arnaud.marant@rte-france.com>
|
|
Could you separate this work into 3 different PRs please:
We think with @SlimaneAmar that it will be easier to follow and merge then. |
|
Thanks for the suggestion. After reviewing the dependency relationships and the associated code changes, I am not sure splitting this work into three PRs would provide much additional value. import { testLayer } from '@deck.gl/test-utils/vitest';This entry point does not exist in DeckGL 9.2. The only application-code adaptation associated with the DeckGL upgrade is a two-line coordinate-system update: getProximityFactor(...) {
- if (this.props.coordinateSystem === COORDINATE_SYSTEM.CARTESIAN) {
+ if (this.props.coordinateSystem === 'cartesian') {
// Cartesian distance calculation
}
}computeAngle(...) {
- if (props.coordinateSystem === COORDINATE_SYSTEM.CARTESIAN) {
+ if (props.coordinateSystem === 'cartesian') {
// Cartesian angle calculation
}
}DeckGL 9.3 changes coordinate systems from numeric values to string values and recommends using the string constants directly. Apart from these two lines, the DeckGL bump is only there to make the new Vitest layer harness available. - "@vitest/coverage-v8": "^4.0.18",
+ "@vitest/coverage-v8": "^4.1.9",- "vitest": "^4.0.18"
+ "vitest": "^4.1.9"The Playwright provider, browser configuration, screenshots, tests and workflow adaptations are all parts of the same browser-testing feature. Consequently, splitting this into three PRs would result in:
These would be stacked rather than independent PRs, and each would introduce overlapping package-lock.json changes. For that reason, I think keeping the changes together provides a more coherent and testable unit of work. If the main concern is the review size, a more meaningful split could be unit/lifecycle tests versus browser visual tests and CI. If useful, I can also explain the dependency relationships and the rationale for keeping these changes together in person. |




Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
Add tests for map layers
What is the current behavior?
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change or deprecate an API?