Skip to content

Yurii bart fix fix tests setup 01#71

Open
r4m-semeyon wants to merge 6 commits intomasterfrom
yurii-bart-fix-fix-tests-setup-01
Open

Yurii bart fix fix tests setup 01#71
r4m-semeyon wants to merge 6 commits intomasterfrom
yurii-bart-fix-fix-tests-setup-01

Conversation

@r4m-semeyon
Copy link
Copy Markdown
Contributor

No description provided.

@route4me route4me deleted a comment from github-actions bot Dec 24, 2025
@github-actions
Copy link
Copy Markdown

Gemini AI Code Risk & Compatibility Analysis

Status: [OK] Analysis completed successfully

Files analyzed:

  • route4me-csharp-sdk/Route4MeSDKLibrary/DataTypes/V5/Locations/LocationTypeResource.cs
  • route4me-csharp-sdk/Route4MeSDKLibrary/Managers/LocationManagerV5.cs
  • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/AddOnRoutesApi/AddOnRoutesApiTests.cs
  • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/Orders/OrdersTests.cs
  • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/PodWorkflows/PodWorkflowsTests.cs
  • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/SchedulesTests.cs
  • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/Vehicles/VehicleTests.cs
  • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5TestHelper.cs

Analysis Results

(node:2259) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. MaxListeners is 10. Use events.setMaxListeners() to increase limit
(Use node --trace-warnings ... to show where the warning was created)
Here is an analysis of the provided code changes.

Summary

The most significant issue in this changeset is a High severity backward compatibility risk due to API signature and data contract changes in the LocationManagerV5. While the method return types in the signatures remain the same, the returned objects are now unwrapped from a new response class, which is a breaking behavior change for existing clients. The changes also include several improvements to unit tests, such as adding detailed error messages on assertion failures and introducing a new test helper class. However, multiple tests have been disabled with [Ignore] attributes, which may indicate regressions or instability in the codebase.


1. Backward Compatibility Risks

  • Severity: High
  • Files:
    • route4me-csharp-sdk/Route4MeSDKLibrary/DataTypes/V5/Locations/LocationTypeResource.cs
    • route4me-csharp-sdk/Route4MeSDKLibrary/Managers/LocationManagerV5.cs

Details:
A new wrapper class LocationTypeResponse has been introduced, which contains a Data property of type LocationTypeResource. Methods in LocationManagerV5 that previously returned the raw LocationTypeResource (or a Tuple containing it) as received from the API have been modified to deserialize into the new LocationTypeResponse and then return only the nested Data property.

Example Change:

// Before
var result = GetJsonObjectFromAPI<LocationTypeResource>(...);
return result;

// After
var result = GetJsonObjectFromAPI<LocationTypeResponse>(...);
return result?.Data;

Risk:
This is a breaking API change for consumers of the SDK. Code that previously consumed methods like GetLocationType or CreateLocationType and expected the entire response object will now receive only the nested LocationTypeResource object. This will likely cause runtime errors (e.g., NullReferenceException if result is null, or casting issues) and unexpected behavior in client applications. The public contract of these methods has changed in behavior, even if the return type in the signature is the same.

Improvement Suggestion:

  1. Major Version Bump: Since this is a breaking change, it should be accompanied by a major version bump of the SDK package (e.g., SemVer).
  2. Clear Release Notes: The change must be clearly and prominently documented in the project's CHANGELOG.md to inform users about the breaking change.
  3. Alternative (Non-Breaking): To avoid a breaking change, new methods could have been introduced, and the old ones marked as [Obsolete]. However, given this is a V5 manager, introducing breaking changes may be acceptable under its versioning strategy.

2. Coding Standard Violations & Potential Risks

  • Severity: Low
  • Files:
    • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/AddOnRoutesApi/AddOnRoutesApiTests.cs
    • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/Orders/OrdersTests.cs
    • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/Vehicles/VehicleTests.cs

Details:
Several unit tests have been disabled using the [Ignore("...")] attribute.

  • DuplicateRoutesTest is ignored due to a "route is not available error".
  • UpdateRouteTest is ignored due to a "500 server error".
  • OrdersArchiveTest and OrdersArchiveAsyncTest are ignored because "Test data haven't prepared".
  • ExecuteVehicleOrder is ignored due to an "unknown server error".

Risk:
Disabling tests can hide regressions or bugs in the codebase. While sometimes necessary temporarily, it reduces test coverage and allows defects to go undetected. The server errors indicate potential instability in the backend API or environment, which could affect production users.

Improvement Suggestion:
Create tickets or work items to investigate the root causes of these test failures and re-enable them as soon as possible. The test data issues should be resolved to ensure the archiving functionality is properly tested.


3. Code Quality Improvements

  • Severity: Low (Positive Change)
  • Files:
    • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5TestHelper.cs
    • All test files (...Tests.cs).

Details:

  1. A new helper class, V5TestHelper, has been introduced to format error messages from ResultResponse objects. This centralizes logic and improves code reusability within the test project.
  2. Assertions in unit tests have been updated to use this helper, providing much more descriptive failure messages.

Example Change:

// Before
Assert.NotNull(result);

// After
var result = route4Me.DeleteRoutes(routeIDs, out var r);
Assert.NotNull(result, V5TestHelper.GetAllErrorMessagesFormatted(r));

Assessment:
This is an excellent improvement. It enhances the maintainability of the tests and significantly speeds up debugging by providing immediate context when a test fails. This is a good coding practice.

@github-actions
Copy link
Copy Markdown

Gemini AI Code Risk & Compatibility Analysis

Status: [OK] Analysis completed successfully

Files analyzed:

  • route4me-csharp-sdk/Route4MeSDKLibrary/DataTypes/V5/Locations/LocationTypeResource.cs
  • route4me-csharp-sdk/Route4MeSDKLibrary/Managers/LocationManagerV5.cs
  • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/AddOnRoutesApi/AddOnRoutesApiTests.cs
  • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/Orders/OrdersTests.cs
  • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/PodWorkflows/PodWorkflowsTests.cs
  • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/SchedulesTests.cs
  • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/Vehicles/VehicleTests.cs
  • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5TestHelper.cs

Analysis Results

Hook registry initialized with 0 hook entries
(node:2385) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. MaxListeners is 10. Use events.setMaxListeners() to increase limit
(Use node --trace-warnings ... to show where the warning was created)
Here is an analysis of the provided code changes.

Backward Compatibility Risks

  • Severity: High
  • Files:
    • route4me-csharp-sdk/Route4MeSDKLibrary/DataTypes/V5/Locations/LocationTypeResource.cs
    • route4me-csharp-sdk/Route4MeSDKLibrary/Managers/LocationManagerV5.cs
  • Risk: The introduction of the LocationTypeResponse class, which wraps LocationTypeResource within a Data property, constitutes a significant breaking change. The LocationManagerV5 methods have been updated to expect this new nested JSON structure from the API ({ "data": { ... } }). Previously, the SDK expected the LocationTypeResource object directly.
  • Impact: Clients using this updated SDK against an older version of the Route4Me API that still returns the flat object structure will encounter runtime errors, as the JSON deserialization will fail to populate the new LocationTypeResponse wrapper object correctly. This will likely result in NullReferenceException when the code attempts to access result.Data.
  • Suggestion: Such a breaking change in the expected API response contract should be accompanied by a major version bump of the SDK (e.g., from v1.x to v2.0.0) according to semantic versioning principles. The release notes must clearly and prominently state that this version requires a newer version of the Route4Me API.

Coding Standards and Best Practices

  • Severity: Low

  • Files:

    • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5TestHelper.cs
    • Various test files (AddOnRoutesApiTests.cs, OrdersTests.cs, etc.)
  • Observation: A new helper class V5TestHelper has been introduced to format error messages from ResultResponse objects. This helper is used throughout the tests to provide more descriptive failure messages in assertions (e.g., Assert.NotNull(result, V5TestHelper.GetAllErrorMessagesFormatted(r))).

  • Suggestion: This is a good improvement. It enhances test maintainability by providing clear, immediate feedback on why a test failed, which speeds up debugging. The implementation of the helper itself is clean and safe.

  • Severity: Low

  • Files:

    • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/AddOnRoutesApi/AddOnRoutesApiTests.cs
    • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/Orders/OrdersTests.cs
    • route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/Vehicles/VehicleTests.cs
  • Observation: Multiple tests have been disabled using the [Ignore] attribute with reasons such as "Failed with 500 server error" or "Test data haven't prepared".

  • Suggestion: While sometimes necessary, ignoring tests can mask regressions or API instability. It is advisable to create technical debt tickets to investigate and fix these tests. A stable and reliable test suite is crucial for maintaining code quality.

No Risks Identified

No significant security risks, performance issues, or deprecated API usage were found in the provided code changes. The modifications are primarily focused on adapting to an API change and improving the quality and robustness of the unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants