[ADMINAPI-1298] - Copy over and enable the execution of the integration tests for Admin…#368
Conversation
Test Results336 tests 335 ✅ 15s ⏱️ Results for commit a2f0c27. ♻️ This comment has been updated with latest results. |
DavidJGapCR
force-pushed
the
ADMINAPI-1298
branch
from
12e4e4b to
f00e935
Compare
DavidJGapCR
force-pushed
the
ADMINAPI-1298
branch
3 times, most recently
from
38ac1a9 to
0204d12
Compare
DavidJGapCR
force-pushed
the
ADMINAPI-1298
branch
from
0204d12 to
ba75641
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR integrates V1 Admin API integration tests into the build process by copying tests from the previous version and updating build scripts to support multiple ODS versions (6.x and 7.x) with different database deployment configurations.
Key Changes:
- Added support for running integration tests against both ODS 6.x and 7.x versions with version-specific parameters
- Introduced mandatory
DbDeployVersionandStandardVersionparameters throughout the database management pipeline - Copied and adapted V1 integration test suite with namespace corrections and modern C# syntax updates
Reviewed Changes
Copilot reviewed 88 out of 88 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| eng/database-manager.psm1 | Enhanced to support multiple DbDeploy versions and made version parameters mandatory |
| build.ps1 | Added dual API version support (6.x and 7.x) with separate test functions |
| EdFi.Ods.AdminApi.V1/Infrastructure/Services/* | Fixed namespace inconsistencies from ClaimSetEditor to Services.ClaimSetEditor |
| EdFi.Ods.AdminApi.V1.DBTests/* | Added complete integration test suite with test infrastructure |
| Ed-Fi-ODS-AdminApi.sln | Added V1 DBTests project to solution |
Comments suppressed due to low confidence (1)
build.ps1:1
- The ValidateSet now includes both versioned and unversioned package names. Consider documenting why 'EdFi.Suite3.RestApi.Databases' (without version) is valid and when it should be used versus the versioned variant.
# SPDX-License-Identifier: Apache-2.0
eng/database-manager.psm1
Outdated
| NuGetFeed = $NuGetFeed | ||
| } | ||
|
|
||
| Write-Output "CALLING ---- Install-EdFiDbDeploy" -ForegroundColor Red |
There was a problem hiding this comment.
This debug output should be removed or replaced with appropriate logging. The -ForegroundColor Red parameter is being used with Write-Output which doesn't support color formatting - use Write-Host instead if color output is needed for debugging.
eng/database-manager.psm1
Outdated
| StandardVersion = $StandardVersion | ||
| } | ||
|
|
||
| Write-Output "CALLING ---- Install-EdFiDatabase" -ForegroundColor Red |
There was a problem hiding this comment.
This debug output should be removed or replaced with appropriate logging. The -ForegroundColor Red parameter is being used with Write-Output which doesn't support color formatting - use Write-Host instead if color output is needed for debugging.
eng/database-manager.psm1
Outdated
| Write-Output "Get-RestApiPackage result:" | ||
| Write-Output " dbPackagePath = $dbPackagePath" |
There was a problem hiding this comment.
These debug outputs should be removed or replaced with appropriate logging mechanisms for production code.
eng/database-manager.psm1
Outdated
| $DbDeployExe = $DbDeployExe -replace "\.exe$", "" | ||
| } | ||
|
|
||
| Write-Host "Full command: $DbDeployExe $($arguments -join ' ')" -ForegroundColor blue |
There was a problem hiding this comment.
This debug output should be removed or replaced with appropriate logging for production code.
| [ | ||
| new() { | ||
| NamespacePrefix = "http://testvendor.net", | ||
| Vendor = new Vendor() // Assign a new Vendor instance to satisfy the required property |
There was a problem hiding this comment.
The workaround comment indicates a code smell. The VendorNamespacePrefix.Vendor property should not require assignment during initialization if it's meant to be a navigation property. Consider reviewing the model design to avoid circular references in initialization.
| [ | ||
| new() { | ||
| NamespacePrefix = "http://testvendor.net", | ||
| Vendor = new Vendor() // Assign a new Vendor instance to satisfy the required property |
There was a problem hiding this comment.
The workaround comment indicates a code smell. The VendorNamespacePrefix.Vendor property should not require assignment during initialization if it's meant to be a navigation property. Consider reviewing the model design to avoid circular references in initialization.
DavidJGapCR
force-pushed
the
ADMINAPI-1298
branch
from
012223f to
a2f0c27
Compare
3 participants
… Api V1 using the build.ps script
Copied over the integration tests from Admin Api v1.
Adjustments on the PowerShell scripts: build.ps1 and its psm1 modules.
Some fixes on namespaces