Clean up AssemblyInfo file updating #1249

bording · 2017-07-01T04:53:47Z

This is still WIP, but here's what I have so far:

AssemblyInfoFileUpdate has been moved into GitVersionCore and renamed AssemblyInfoFileUpdater
The UpdateAssemblyInfo task has been rewritten to use AssemblyInfoFileUpdater

Things to do still:

Add a new way to generate the GitVersionInformation class
Remove the AssemblyInfoBuilder stuff from GitVersionTask
Look at the existing templates and see what changes might be needed
Figure out which of the other issues linked here should be fixed as part of this PR and which should be handled in subsequent PRs

Fixes #1242

bording · 2017-07-01T06:17:08Z

I see there there are some test failures, but none of them look related to my changes...

bording · 2017-07-07T04:36:35Z

Made a bit more progress. I've turned AssemblyVersionInfoTemplates into a more general purpose TemplateManager class, and I'm getting the add formats from embedded resources as well.

I've also started the GitVersionInformationGenerator implementation, which uses the TemplateManager to load its templates from embedded resources as well.

asbjornu

I really like what this PR does. Please have a look at my comments and let me know what you think.

asbjornu · 2017-07-10T09:05:59Z

src/GitVersionCore/GitVersionCore.csproj

@@ -119,6 +119,9 @@
    <Compile Include="GitVersionCache.cs" />
    <Compile Include="GitVersionCacheKey.cs" />
    <Compile Include="GitVersionCacheKeyFactory.cs" />
+    <EmbeddedResource Include="GitVersionInformationResources\Templates\GitVersionInformation.cs" />
+    <EmbeddedResource Include="GitVersionInformationResources\AddFormats\GitVersionInformation.cs" />


Since these are jumbled around anyway, would you mind grouping the related files together in the .csproj?

asbjornu · 2017-07-10T11:13:31Z

src/GitVersionCore.Tests/GitVersionCore.Tests.csproj

-    <Reference Include="Shouldly, Version=2.7.0.0, Culture=neutral, PublicKeyToken=6042cbcb05cbc941, processorArchitecture=MSIL">
-      <HintPath>..\packages\Shouldly.2.7.0\lib\net40\Shouldly.dll</HintPath>
-      <Private>True</Private>
+    <Reference Include="Shouldly, Version=2.8.3.0, Culture=neutral, PublicKeyToken=6042cbcb05cbc941, processorArchitecture=MSIL">


Since some of the build failures seems related to the upgrade of Shoudly, can you perhaps avoid that upgrade in this PR? On one of the Travis builds, this is the output:

The type initializer for Shouldly.ShouldlyConfiguration threw an exception.

I can back this out, but the reason I upgraded it was that the older version was throwing an error about it not having a diff tool, so I was blocked. Upgrading it allowed it to use the VS diff tool.

I would appreciate if you could install an external diff tool so we could have this PR free of any package upgrades. I hope that should take care of the build failures.

@asbjornu I do have an external diff tool, SourceGear DiffMerge, but Shouldly doesn't appear to know about it. Are there some docs I can follow to temporarily wire that up so I can actually run the tests but not need to bump the version?

Seems like this might be a good place to start.

Thanks, I'll see what I can do to get it working and I'll remove the package upgrade.

bording · 2017-07-11T03:17:19Z

@asbjornu I have cleaned up the project files and removed the Shouldly update, but it looks like AppVeyor is still failing. I've run all of those failing tests locally, and they seem to work fine.

Regarding the rest of the changes to finish this PR up, I hope to have it wrapped up this week.

asbjornu · 2017-07-11T14:09:40Z

@bording: Those build failures are indeed very strange. If someone else from @GitTools/developers should come by, here's the failure:

Error : ConfigProviderTests.CanWriteOutEffectiveConfiguration
System.Exception : Cannot find method in call stack with attribute NUnit.Framework.TestCaseAttribute
   at Shouldly.Configuration.FindMethodUsingAttribute`1.GetTestMethodInfo(StackTrace stackTrace, Int32 startAt) in C:\projects\shouldly\src\Shouldly.Shared\Configuration\FindMethodUsingAttribute.cs:line 20
   at Shouldly.ShouldMatchApprovedTestExtensions.ShouldMatchApproved(String actual, Func`1 customMessage, Action`1 configureOptions) in C:\projects\shouldly\src\Shouldly.Shared\ShouldlyExtensionMethods\ShouldMatchApprovedTestExtensions.cs:line 49
   at Shouldly.ShouldMatchApprovedTestExtensions.ShouldMatchApproved(String actual) in C:\projects\shouldly\src\Shouldly.Shared\ShouldlyExtensionMethods\ShouldMatchApprovedTestExtensions.cs:line 17
   at ConfigProviderTests.CanWriteOutEffectiveConfiguration() in C:\projects\gitversion\src\GitVersionCore.Tests\ConfigProviderTests.cs:line 177

bording · 2017-07-16T04:33:28Z

Pushed up a few more changes, but didn't get as much done as I'd hoped I would. 😞

@asbjornu I started looking into turning the existing AssemblyInfoBuilder tests in GitVersionTask.Tests tests into new GitVersionInformationGenerator tests, but they're a lot more involved than I was expecting. In addition to using Shouldly to write the file output to disk, it's also compiling the code with Rosyln and loading the assembly into memory to test it.

Is it really important to keep this aspect in the new tests? It seems a bit excessive to me. Also, it only tests the C# and VB output, and not F#. I'm thinking that having the Shouldly output to verify the output looks correct would be enough here, especially considering that's all the AssemblyInfoFileUpdater tests do.

bording · 2017-07-16T04:37:13Z

Hmm, still seeing those weird test failures. I see that other PRs have been merged recently, and the failures didn't happen on those branches, but I'm not sure what I've done here that would cause them. I guess I'll need to spend some time figuring that out since it doesn't look like it's going away on it's own...

asbjornu · 2017-07-16T16:49:17Z

Compiling the generated files with Roslyn is a bit excessive, I agree. It's good to know that the generated content actually compiles (especially since it's just basic and unsafe string replacements), but we can probably move that to the build pipeline instead. @gep13, do you have Ideas on how we can have Cake do the instrumentation there instead of doing it within a test?

It's unfortunate that the tests are failing and that I don't have time assisting you. I'm back from holiday in a couple of weeks and might be able to take a closer look then. The best of luck meanwhile, though and thanks for this work, @bording! It's highly appreciated!

bording · 2017-08-31T03:31:47Z

Just wanted to chime in and say that I'm still planning on getting back to finishing this. Lack of spare time and then vacation has conspired to prevent me from working on it recently.

Hope to get back to it soon!

bording · 2017-10-08T01:38:58Z

@asbjornu With the changes I've just pushed up, I'm removing the WIP and think this is ready for a review.

I haven't added a way to create the GitVersionInformation file from GitVersionExe yet, but that wasn't something it could do before anyway, so it could be added as a followup PR since this one is large enough as-is.

bording · 2017-10-08T01:50:07Z

Looks like those unrelated tests are still failing on AppVeyor 😢

asbjornu · 2017-10-09T22:19:18Z

@bording: Thanks for your work on this! ❤️ Those failing tests are bizarre. Especially because they're only failing on AppVeyor. Perhaps an upgrade of Shouldly can help?

bording · 2017-10-09T22:27:09Z

@bording: Thanks for your work on this! ❤️ Those failing tests are bizarre. Especially because they're only failing on AppVeyor. Perhaps an upgrade of Shouldly can help?

I can try it and see if it makes a difference.

dazinator · 2017-10-09T23:44:24Z

Quick look at the stack trace a few comments above, nunit attribute is mentioned - so could also check if updating nunit packages helps at all.

bording · 2017-10-10T00:39:06Z

Dug into the Shouldly code a bit and I realized what was going on! The tests I added were calling ShouldlyConfiguration.ShouldMatchApprovedDefaults.LocateTestMethodUsingAttribute<TestCaseAttribute>();, but the failing tests were all using Test, not TestCase.

The configuration appears to be global, so I added ShouldlyConfiguration.ShouldMatchApprovedDefaults.LocateTestMethodUsingAttribute<TestAttribute>(); to the tests that were failing, and now everything is passing!

And, since those tests were all marked to not run on mono, that's why Travis wasn't failing.

@asbjornu Should I revert the Shouldly updates? Maybe go back to 2.8.3 instead of the beta?

asbjornu · 2017-10-10T05:12:13Z

Excellent! 2.8.3 sounds fine.

bording · 2017-10-10T16:06:21Z

Excellent! 2.8.3 sounds fine.

Done!

bording · 2017-10-10T16:47:10Z

Sigh, now it looks like 2.8.3 is causing Travis to fail because it added a Visual Studio difftool option, and looking for that is throwing.

I guess back to 2.7.0 it is!

asbjornu · 2017-10-12T11:35:22Z

@bording: Fantastic work, thank you so much for this! ❤️

asbjornu requested changes Jul 10, 2017

View reviewed changes

bording added 21 commits October 7, 2017 16:58

Move AssemblyInfoFileUpdate into GitVersionCore

724f2fa

Remove dependency on Arguments

b84d0ef

Move tests

f22920e

Fix Tests

cdacfc3

Rename class to AssemblyInfoFileUpdater

8185eea

Clean up AssemblyInfoFileUpdater and simplify tests

a1ff7df

Use AssemblyInfoFileUpdater in UpdateAssemblyInfo

ef8bd0a

Rename DoNotRestoreAssemblyInfo to CommitChanges

831da90

Remove unneeded initialization of required property

239ae75

Remove unneeded using statement

057fcb5

Replace AssemblyVersionInfoTemplates with TemplateManager

0455e0b

Add initial GitVersionInformationGenerator implementation

9f63545

Clean up project files

d3d2eea

Make TemplateManager use file extension for GetTemplateFor

f5e8828

Use non-temp parameter name

36e5787

Ensure TemplateManager always ignores file extension case

9e5b327

Formatting

f76e977

Add validation to public methods

e55b2e5

Move UpdateAssemblyInfo out of AssemblyInfoBuilder folder

c0997dd

Fix project file after rebase

fc6bf87

Update ShouldCreateFile test

043ba85

bording added 3 commits October 7, 2017 21:32

Remove AssemblyInfoBuilder and tests

c38c9c0

Add GenerateGitVersionInformation task

6ef455f

Template tweaks

51760a6

bording changed the title ~~[WIP] Clean up AssemblyInfo file updating~~ Clean up AssemblyInfo file updating Oct 8, 2017

Remove unused packages from GitVersionTask.Tests

af16ddb

Fix failing tests

227d720

asbjornu approved these changes Oct 12, 2017

View reviewed changes

asbjornu merged commit 0678efa into GitTools:master Oct 12, 2017

bording deleted the assemblyInfo branch October 12, 2017 12:58

bording mentioned this pull request Nov 5, 2017

#1175 - NetStandard #1269

Merged

asbjornu mentioned this pull request Mar 1, 2019

When version attributes missing when using exe? #529

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up AssemblyInfo file updating #1249

Clean up AssemblyInfo file updating #1249

bording commented Jul 1, 2017 •

edited

Loading

bording commented Jul 1, 2017

bording commented Jul 7, 2017

asbjornu left a comment

asbjornu Jul 10, 2017

asbjornu Jul 10, 2017

bording Jul 10, 2017

asbjornu Jul 10, 2017

bording Jul 10, 2017

asbjornu Jul 10, 2017

bording Jul 10, 2017

bording commented Jul 11, 2017

asbjornu commented Jul 11, 2017

bording commented Jul 16, 2017

bording commented Jul 16, 2017

asbjornu commented Jul 16, 2017

bording commented Aug 31, 2017

bording commented Oct 8, 2017

bording commented Oct 8, 2017

asbjornu commented Oct 9, 2017

bording commented Oct 9, 2017

dazinator commented Oct 9, 2017

bording commented Oct 10, 2017

asbjornu commented Oct 10, 2017

bording commented Oct 10, 2017

bording commented Oct 10, 2017

asbjornu commented Oct 12, 2017

Clean up AssemblyInfo file updating #1249

Clean up AssemblyInfo file updating #1249

Conversation

bording commented Jul 1, 2017 • edited Loading

bording commented Jul 1, 2017

bording commented Jul 7, 2017

asbjornu left a comment

Choose a reason for hiding this comment

asbjornu Jul 10, 2017

Choose a reason for hiding this comment

asbjornu Jul 10, 2017

Choose a reason for hiding this comment

bording Jul 10, 2017

Choose a reason for hiding this comment

asbjornu Jul 10, 2017

Choose a reason for hiding this comment

bording Jul 10, 2017

Choose a reason for hiding this comment

asbjornu Jul 10, 2017

Choose a reason for hiding this comment

bording Jul 10, 2017

Choose a reason for hiding this comment

bording commented Jul 11, 2017

asbjornu commented Jul 11, 2017

bording commented Jul 16, 2017

bording commented Jul 16, 2017

asbjornu commented Jul 16, 2017

bording commented Aug 31, 2017

bording commented Oct 8, 2017

bording commented Oct 8, 2017

asbjornu commented Oct 9, 2017

bording commented Oct 9, 2017

dazinator commented Oct 9, 2017

bording commented Oct 10, 2017

asbjornu commented Oct 10, 2017

bording commented Oct 10, 2017

bording commented Oct 10, 2017

asbjornu commented Oct 12, 2017

bording commented Jul 1, 2017 •

edited

Loading