-
Notifications
You must be signed in to change notification settings - Fork 449
fix: matrix load tests #2063
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: matrix load tests #2063
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request fixes matrix load testing by refactoring the test classes and the matrix API request creation logic, along with improving configuration handling and enhancing error messages.
- Updated test cases for matrix request body creation and coordinate/integer parsing
- Refactored MatrixAlgorithmLoadTest and related API endpoints to use MatrixModes instead of DirectionsModes
- Added overloads and documentation updates to streamline error handling and internal configuration
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
MatrixAlgorithmLoadTestTest.java | Added comprehensive tests to cover request body creation and input parsing scenarios |
BenchmarkEnumsTest.java | Extended tests for new MatrixModes conversion and parameter retrieval |
RequestBodyCreationException.java | Added an additional constructor to support single parameter exceptions |
MatrixAlgorithmLoadTest.java | Refactored methods to parse CSV inputs and create matrix API requests using MatrixModes |
Config.java | Updated to include separate configuration for matrixModes, though an error exists in its implementation |
BenchmarkEnums.java | Added MatrixModes enum with dedicated request parameters and profiles |
Comments suppressed due to low confidence (1)
ors-benchmark/src/main/java/org/heigit/ors/benchmark/Config.java:159
- The getMatrixModes method incorrectly processes the 'directionsModes' property instead of using the 'matrixModes' property. Update the stream to use 'matrixModes.stream()' to ensure the correct mode configuration is applied.
: directionsModes.stream().map(MatrixModes::fromString).toList();
case "algodijkstra" -> ALGO_DIJKSTRA_MATRIX; | ||
case "algocore" -> ALGO_CORE_MATRIX; | ||
case "algorphast" -> ALGO_RPHAST_MATRIX; | ||
default -> throw new IllegalArgumentException("Invalid directions mode: " + value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message in MatrixModes.fromString refers to 'directions mode' instead of 'matrix mode'. Consider updating it to 'Invalid matrix mode: ' to better reflect the context.
default -> throw new IllegalArgumentException("Invalid directions mode: " + value); | |
default -> throw new IllegalArgumentException("Invalid matrix mode: " + value); |
Copilot uses AI. Check for mistakes.
1fed439
to
1b8edbb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 👍
Please consider addressing the two issues reported by SonarQube, and the one by Copilot. Cheers!
1b8edbb
to
4cdb54e
Compare
Addressed all comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request fixes matrix load testing by introducing new tests and updating the implementation to use MatrixModes. Key changes include:
- Adding comprehensive tests for matrix load request body creation and error handling.
- Updating load test logic and configuration to use MatrixModes instead of DirectionsModes.
- Enhancing BenchmarkEnums with a new MatrixModes enum and accompanying request parameter methods.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
ors-benchmark/src/test/java/org/heigit/ors/benchmark/MatrixAlgorithmLoadTestTest.java | Added tests covering valid and error scenarios for matrix load request bodies. |
ors-benchmark/src/test/java/org/heigit/ors/benchmark/BenchmarkEnumsTest.java | Added tests for the MatrixModes enum functionality. |
ors-benchmark/src/main/java/org/heigit/ors/benchmark/exceptions/RequestBodyCreationException.java | Added an overloaded constructor to support exception messages. |
ors-benchmark/src/main/java/org/heigit/ors/benchmark/MatrixAlgorithmLoadTest.java | Updated load test logic and request body creation to support MatrixModes. |
ors-benchmark/src/main/java/org/heigit/ors/benchmark/Config.java | Updated configuration to parse and support new matrix mode settings. |
ors-benchmark/src/main/java/org/heigit/ors/benchmark/BenchmarkEnums.java | Introduced MatrixModes with associated request parameters and profile retrieval. |
Comments suppressed due to low confidence (1)
ors-benchmark/src/main/java/org/heigit/ors/benchmark/BenchmarkEnums.java:98
- The getProfiles() method for MatrixModes returns only 'driving-car', but tests expect additional profiles (e.g., 'foot-walking') and a total of 4 profiles. Consider updating getProfiles() to return the complete set of expected profiles.
return switch (this) { case ALGO_DIJKSTRA_MATRIX, ALGO_CORE_MATRIX, ALGO_RPHAST_MATRIX -> List.of("driving-car");//, "driving-hgv", "cycling-regular", "foot-walking"); };
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for addressing the issues pointed out in my previous review 👍
Changes from the recent commits raised a few new SonarQube issues, plus a seemingly valid comment from Copilot. Would you mind looking into these? Cheers! ❤️
48f3d0f
to
1c20680
Compare
Fixed sonarqube issues. What copilot mentioned I fixed as well, but I opted to go for vehicle-car benchmarking for now only. |
|
Pull Request Checklist
have been resolved.
[Unreleased] heading.
along with a short description of what it is for, and documented this in the Pull Request (below).
(at least Germany), and the graphs build without problems (i.e. no out-of-memory errors).
importer etc.), I have generated longer distance routes for the affected profiles with different options
(avoid features, max weight etc.) and compared these with the routes of the same parameters and start/end
points generated from the current live ORS.
If there are differences then the reasoning for these MUST be documented in the pull request.
and why the change was needed.
Fixes matrix load testing.
Information about the changes
Examples and reasons for differences between live ORS routes, and those generated from this pull request
Required changes to ors config (if applicable)