Skip to content

Run UnitsNet JSON serialization tests on the latest UnitsNet libraries in Nuget #423

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

Closed
wants to merge 3 commits into from
Closed

Run UnitsNet JSON serialization tests on the latest UnitsNet libraries in Nuget #423

wants to merge 3 commits into from

Conversation

gojanpaolo
Copy link
Contributor

@gojanpaolo gojanpaolo commented Mar 24, 2018

Fixes #417

The main idea is to use build configuration to switch the UnitsNet reference of the serialization test project:
If in debug configuration, use the UnitsNet.Serialization project reference.
If in release configuration, use the latest version of UnitsNet.Serialization as well as UnitsNet in nuget.

I tested this by changing the Version to "1.2.2" ("*" in the commit). I then ran the build.bat file and it failed on some of the serialization tests as expected.

Hopefully this doesn't conflict with anything in the Appveyor. I'm not really familiar with that tool yet.

For the serialization test project:
If in debug configuration, use the UnitsNet.Serialization project reference.
If in release configuration, use the latest version of UnitsNet.Serialization as well as UnitsNet in nuget.
@gojanpaolo gojanpaolo changed the title Use build configuration to switch reference Run UnitsNet JSON serialization tests on the latest UnitsNet libraries in Nuget Mar 24, 2018
@angularsen
Copy link
Owner

Interesting approach, a couple of thoughts:

  1. The PR as it stands, with version *, does not help us catch any errors - right? We would have to change it to 1.2.2 or similar?
  2. With 1.2.2 set, we only get test errors when running tests locally (typically a debug build). We should find a way to get broken builds on AppVeyor in order to get feedback on pull requests when making breaking changes to either UnitsNet or the json lib, and the only way I see we can accomplish that is to have AppVeyor build and test BOTH debug and release configurations. It's doable, but it will take twice the time to run and it generally does not feel like the "right solution" to the problem.

Did you look into the resources in #417 regarding side-loading multiple versions of the same library? One "latest" csproj build and one v1.2.2 UnitsNet.dll version copied into the repository?
I think that should work, I just haven't looked more closely into it.

@gojanpaolo
Copy link
Contributor Author

Version="*" will get the latest nuget version so we dont need to change it. I think we also need to add a conditional ProjectReference for the main UnitsNet library for the release configuration.

So when in release config, this will catch if the serialization library needs to be updated along with the unitsnet library:

  1. Dev makes a change on the unitsnet library which the latest serialization library in nuget cannot serialize.
  2. Dev commits
  3. Appveyor runs the tests in release config
  4. Test library will reference the latest serialization library in nuget but will reference the main unitsnet project.
  5. Tests in Appveyor will fail.

Yes, I looked at the resources in #417 but this solution came into mind. :)

@gojanpaolo
Copy link
Contributor Author

I'll make another commit later. Then here's how i plan to test it:

  1. Create another branch from this branch
  2. Make change to the unitsnet project that will break serialization in nuget
  3. Commit
  4. See Appveyor fail as expected.

I'll let you know the results. :)

@angularsen
Copy link
Owner

angularsen commented Mar 24, 2018

Ah, I think I got it. I do think your thinking is good, but not sure if the code changes will actually work this way. The test project only references the JSON lib and transitively gets the UnitsNet lib, based on what the JSON lib in turn references.

So, if I got it right:

  1. When developing and building locally with Debug config, json tests run with latest JSON nuget - which in turn brings in the latest UnitsNet nuget. If you introduced breaking changes in your branch's UnitsNet code, that UnitsNet code will not be tested - only the latest published UnitsNet nuget.
  2. When AppVeyor builds with Release config, it references the branch's JSON code - which in turn references the branch's UnitsNet code - so if you made a breaking change in UnitsNet and also added support for that in the JSON branch - all tests will be green. We still don't know if it broke compatibility with an older JSON lib version. This is how it already works today.

@gojanpaolo gojanpaolo mentioned this pull request Mar 24, 2018
@gojanpaolo
Copy link
Contributor Author

gojanpaolo commented Mar 24, 2018

The test project only references the JSON lib and transitively gets the UnitsNet lib

Adding the conditional ProjectReference for the UnitsNet should solve this. See commit 3bafcab

It's the other way around.

  1. When developing and building locally with Debug config, json tests run with the branch's UnitsNet and JSON code.

    If you introduced breaking changes in your branch's UnitsNet code, that UnitsNet code will be tested - so the json tests will fail. You will then update the JSON code to support this until the tests pass.

  2. When AppVeyor builds with Release config, it references the latest JSON nuget and the branch's UnitsNet code - so if you made a breaking change in UnitsNet and also added support for that in the JSON branch - all or some tests will fail which means that the JSON nuget is not compatible anymore.

@angularsen
Copy link
Owner

angularsen commented Mar 24, 2018

Aha, yes I got it the wrong way around. So by introducing the extra reference in Release, you force it to test the latest released JSON nuget against the current UnitsNet code - nice! That should indeed help catch breaking changes against latest JSON nuget - BUT - when working on new JSON features/changes, AppVeyor will not run tests on those new changes.

If I'm not mistaken, we need to run tests against two versions of JSON lib to cover both scenarios:

  1. Latest JSON nuget + local UnitsNet code to catch breaking changes in UnitsNet
  2. Local JSON code + local UnitsNet code to catch functional errors in new JSON code

@gojanpaolo
Copy link
Contributor Author

gojanpaolo commented Mar 24, 2018

Local JSON code + local UnitsNet code to catch functional errors in new JSON code

Maybe we can also make the AppVeyor run the tests in Debug config?

Also, for the Release config, will it be better to force Assert.Inconclusive (yellow) if the JSON nuget fails? This way, we can easily distinguish it from the branch's code test fails (red).

@gojanpaolo
Copy link
Contributor Author

gojanpaolo commented Mar 25, 2018

I think we can make custom build configurations:

https://stackoverflow.com/questions/6051054/how-do-i-pass-a-property-to-msbuild-via-command-line-that-could-be-parsed-into-a?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa

https://stackoverflow.com/questions/606660/does-msbuild-recognise-any-build-configurations-other-than-debugrelease

https://docs.microsoft.com/en-us/visualstudio/ide/how-to-create-and-edit-configurations

So maybe just create a custom configuration (e.g. TestJsonNuget) instead of Debug & Release. and then use that as the switch? I believe it has the following pros:

  1. More explicit on what we're trying to accomplish.
  2. AppVeyor will test both the branch's code in Release mode.
  3. We may need to use the Debug/Release config later on which may conflict with what we're trying to accomplish here.

@angularsen
Copy link
Owner

angularsen commented Mar 25, 2018

Hm. I just realized the easiest way to achieve this is to build and run the JSON tests twice. Doesn't really matter if we use Debug/Release, TestJsonNuget or some other way to distinguish - we still need to do the whole build/run tests cycle twice. The only difference is what libraries are linked in.

In that light, there is a third option:

  • Create a new JSON test project whose purpose is to test backwards compatibility (latest JSON nuget against current UnitsNet code)
  • Let the original test project test the current JSON code against the current UnitsNet code, as it already does

The advantage of this is that you only have to "build once" and not mess with build parameters to control this. You simply have two test projects with different references, running the same test code.

The new csproj format and the dotnet CLI tooling is a bit icky in terms of sharing source code for two csproj projects. dotnet test is confused when there are multiple test projects in the same folder, so you need to put them in separate folders and write custom wildcard <Include> in order to link in all the JSON test code from a different folder. The folder structure would be something like this:

|- UnitsNet.Serialization.JsonNet.Tests
|- UnitsNet.Serialization.JsonNet.CompatibilityTests

@gojanpaolo
Copy link
Contributor Author

Haha. Yes I agree that will be an easier and better solution. I'll try make another branch and will submit another PR with that solution. :)

@gojanpaolo
Copy link
Contributor Author

Closing this in favor of #425

@gojanpaolo gojanpaolo closed this Mar 25, 2018
angularsen pushed a commit that referenced this pull request Mar 26, 2018
* Create another test project to run UnitsNet JSON serialization tests on the latest UnitsNet libraries in Nuget

The new test project is `UnitsNet.Serialization.JsonNet.CompatibilityTests`.

UnitsNetJsonConverterTests.cs is added as a link.
`    <Compile Include="..\UnitsNet.Serialization.JsonNet.Tests\UnitsNetJsonConverterTests.cs" Link="UnitsNetJsonConverterTests.cs" />`

The project runs the serialization tests on the latest version of the UnitsNet.Serialization.JsonNet library in Nuget.

`<PackageReference Include="UnitsNet.Serialization.JsonNet" Version="*" />`

* Added xUnit DotNetCliToolReference

* Force compatibility tests to use branch's UnitsNet code

see #423 (comment)

Compatibility test must use JSON Nuget and branch's UnitsNet code

* Added compatibility tests project to Start-Tests script function
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