Remove JSON examples from artifacts #450
Conversation
|
This a stale pull request. Please review, update or/and close as necessary. |
|
This PR conflicts current |
| @@ -15,7 +15,7 @@ namespace CarbonAware.WepApi.IntegrationTests; | |||
| [TestFixture(DataSourceType.JSON)] | |||
| [TestFixture(DataSourceType.WattTime)] | |||
| [TestFixture(DataSourceType.ElectricityMaps)] | |||
| [TestFixture(DataSourceType.ElectricityMapsFree)] | |||
| //[TestFixture(DataSourceType.ElectricityMapsFree)] // TODO: need to implement data source into IntegrationTestingBase.cs | |||
There was a problem hiding this comment.
Do we need to document better why this is being ignored for this PR please?
There was a problem hiding this comment.
I updated my Visual Studio Community 2022 to version 17.12.3 , then all tests are passed without commented-out.
This screenshot shows EMFree datasource tests in both CarbonAwareControllerTests.cs and LocationsControllerTests.cs have been skipped. I wonder why they were skipped, but it might be the issue in VS (or test facilities in dotnet) because the issue has gone in the latest VS.
Can someone evaluate on your environment without // ? I will remove // if it works on other environment(s).
There was a problem hiding this comment.
The tests failed again!
The root cause is that the test for ElectricityMapsFree would load JsonDataSource.
As I pointed before, ElectricityMapsFree has not been handled at IntegrationTestingBase.
WebAPI testcase would use src/CarbonAware.WebApi/src/appsettings.json which makes dependency to JsonDataSource. Thus tests for ElectricityMapsFree works without errors - it means ElectricityMapsFree tests are not valid because it uses JsonDataSource.
@danuw @vaughanknight
Should we fix the tests for ElectricityMapsFree before this PR?
YaSuenag
force-pushed
the
pr/remove-jsons
branch
from
5f0958e to
d245c39
Compare
Closes Green-Software-Foundation#440 Signed-off-by: Yasumasa Suenaga <[email protected]>
YaSuenag
force-pushed
the
pr/remove-jsons
branch
from
d245c39 to
ed28d43
Compare
|
@danuw @vaughanknight Based on discussion in weekly meeting at Jan 28, I included EMFree datasource test in this PR. It works fine on both GHA and Visual Studio on my PC. I believe the final approach was done. Please review! |
|
@vaughanknight any way you can take a look at that PR please? I am happy about the changes though I have not tested but was wondering if I could get a pass from you this week too so we can move this along? - Thanks cc @YaSuenag |
There was a problem hiding this comment.
PR Overview
This PR removes JSON artifacts from the build and updates configurations in both source projects and tests to align with the new behavior. Key changes include:
- Removal of JSON file references from project files so that JSONs are now created in the publish directory.
- Updates to test configuration and environment setups across WebAPI, CLI, and data source tests.
- Adjustments in the CI workflow to set JSON data source related environment variables.
Reviewed Changes
| File | Description |
|---|---|
| src/CarbonAware.WebApi/test/integrationTests/IntegrationTestingBase.cs | Added a new switch case for configuring the ElectricityMapsFree data source. |
| .github/workflows/1-pr.yaml | Updated environment variable settings and run steps related to generating the OpenAPI document and running the container. |
| src/CarbonAware.CLI/test/integrationTests/IntegrationTestingBase.cs | Added JSON data file location environment variable for CLI integration tests. |
| src/CarbonAware.WebApi/test/integrationTests/WebApiEndpointTests.cs | Renamed test fixture and updated data source type for endpoint tests. |
| src/CarbonAware.DataSources/CarbonAware.DataSources.Json/mock/JsonDataSourceMocker.cs | Updated the constructor to set the DataFileName property, used in test setups. |
| src/CarbonAware.DataSources/CarbonAware.DataSources.Json/src/Configuration/JsonDataSourceConfiguration.cs | Removed the default data file setting to reflect the removal of JSON artifacts. |
| src/CarbonAware.DataSources/CarbonAware.DataSources.Json/test/JsonDataSourceConfigurationTests.cs | Removed the test that verified the default JSON file location. |
| src/CarbonAware.DataSources/CarbonAware.DataSources.Json/test/JsonDataSourceTests.cs | Updated tests to use the new configuration set up from the JsonDataSourceMocker. |
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/CarbonAware.WebApi/test/integrationTests/IntegrationTestingBase.cs:120
- [nitpick] The environment variable for the ElectricityMapsFree token uses '__token' while other data sources use a more descriptive key (e.g. '__APIToken'). Consider standardizing the naming for clarity and consistency.
case DataSourceType.ElectricityMapsFree:
.github/workflows/1-pr.yaml:129
- There appears to be a potential duplication of the 'Generate Open API' step with environment variable settings. Verify that the run command is not unintentionally duplicated.
run: dotnet tool run swagger tofile --output ./wwwroot/api/v1/swagger.yaml --yaml ${{ env.DLL_FILE_PATH }} v1
Pull Request
#440
Summary
Remove data source and location source JSONs from artifacts.
These JSONs are no longer provided from both CLI and WebAPI includes container image, but we can use them of course if we need because JSONs are still kept in the repo.
Changes
Essense of this PR is following:
data-sources/fromItemGroup, anddata-sources/jsonwould be created in publish directory.location-sources/fromItemGroup, andlocation-sources/jsonwould be created in publish directory.test-data-azure-emissions.jsonis no longer set by default.dotnet tool runfor generating OpenAPI document.data-sourcesandlocation-sources.data-sourcesandlocation-sources, but they would be created into publish directory.This PR has other changes, but they are for test cases because they depends JSON data source and location source.
Note that I commented out test case for ElectricityMapsFree in both src/CarbonAware.WebApi/test/integrationTests/CarbonAwareControllerTests.cs and src/CarbonAware.WebApi/test/integrationTests/LocationsControllerTests.cs because IntegrationTestingBase.cs does not have configuration for ElectricityMapsFree. I will fix it after this PR.
Checklist
Are there API Changes?
No
Is this a breaking change?
This PR breaks current behavior. CASDK is no longer refer test-data-azure-emissions.json for data source and azure-regions.json for location source. CASDK requires the user to configure data source at least.