Skip to content
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

Allow custom loggers in the VSTestSettings #1138

Closed
Jericho opened this issue Aug 8, 2016 · 3 comments
Closed

Allow custom loggers in the VSTestSettings #1138

Jericho opened this issue Aug 8, 2016 · 3 comments

Comments

@Jericho
Copy link
Contributor

Jericho commented Aug 8, 2016

VSTestSettings has a property called "Logger" with two possible values: None or Trx.

However, we need to be able to specify custom values. For example, when running on AppVeyor we can specify "/logger:AppVeyor".

As a workaround, I was able to specify the AppVeyor logger using ArgumentCustomization like so:

Task("Run-Code-Coverage")
    .IsDependentOn("Build")
    .Does(() =>
{
    var testAssemblyPath = string.Format("./MyLibraryUnitTests/bin/{0}/MyLibrary.UnitTests.dll", configuration);
    var vsTestSettings = new VSTestSettings();
    if (AppVeyor.IsRunningOnAppVeyor) vsTestSettings.ArgumentCustomization = args => args.Append("/logger:Appveyor");

    OpenCover(
        tool => { tool.VSTest(testAssemblyPath, vsTestSettings); },
        new FilePath("./CodeCoverageData/coverage.xml"),
        new OpenCoverSettings()
            .ExcludeByAttribute("*.ExcludeFromCodeCoverage*")
            .WithFilter("+[MyLibrary]*")
            .WithFilter("-[MyLibrary]MyLibrary.Properties.*")
    );
});

A few ideas were suggested:

  • Change the Logger property to a string and create some nice extension methods to accomplish the same thing. Like .WithAppVeyorLogger() or similar (this would probably break existing scripts though).
  • Perhaps a CustomLogger string?
@Jericho
Copy link
Contributor Author

Jericho commented Aug 9, 2016

I propose the following:

  1. Mark the 'Logger' property as obsolete (I don't know what you policy is regarding obsolete properties but I imagine you want to keep it around for a release or two before removing it).
  2. Also mark the VSTestLogger enum as obsolete. It wil no longer be necessary when the 'Logger' property is removed.
  3. Add a new string property called 'LoggerName'
  4. Add a few extension methods such as:
    .WithTrxLogger()
    .WithAppVeyorLogger()
    .WithCustomLogger(string loggerName)
    .WithoutAnyLogger()
  5. As much as possible, keep the obsolete 'Logger' property and the new property in sync (for example: set LoggerName to "trx" if Logger is set to VSTestLogger.Trx; set LoggerName to a blank string if Logger is set to VSTestLogger.None; etc) and throw an exception if, for some reason, the two properties can't be kept in sync.
  6. Add a new value called 'Custom' to the VSTestLogger enum. This will make it a little bit easier to keep the two properties in sync

I would be happy to submit a PR with these changes.

@gep13
Copy link
Member

gep13 commented Aug 9, 2016

@Jericho in general, this seems like a valid approach to me. Question though... Aren't 2 and 6 slightly contradictory?

@Jericho
Copy link
Contributor Author

Jericho commented Aug 9, 2016

They are not contradictory. 6 is necessary to keep the to properties in sync. For example if I invoke .WithCustomLogger("mylogger") I will set the LoggerName to "mylogger" and I also need to be able to set Logger to VSTestLogger.Custom

To be clear, 6 is temporary until we remove the obsolete property and obsolete enum.

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

No branches or pull requests

3 participants