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

Fix aspnet-session failing tests #26

Closed
wants to merge 2 commits into from
Closed

Fix aspnet-session failing tests #26

wants to merge 2 commits into from

Conversation

petemounce
Copy link
Contributor

No description provided.

@petemounce petemounce changed the title Make tests run further now, but still fail :( Fix aspnet-session failing tests Feb 3, 2016
@@ -23,6 +24,7 @@ public AspNetSessionValueLayoutRendererTests()

public void SetUp()
{
ConfigurationItemFactory.Default.LayoutRenderers.RegisterDefinition("aspnet-session", typeof(AspNetSessionValueLayoutRenderer));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be needed if nlog 4.0.1+ is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used both the resharper xunit test runner and the ncrunch test runner, and without that, I got

System.ArgumentException: LayoutRenderer cannot be found: 'aspnet-session'
   at NLog.Config.Factory`2.CreateInstance(String name)
   at NLog.Layouts.LayoutParser.ParseLayoutRenderer(ConfigurationItemFactory configurationItemFactory, SimpleStringReader sr)
   at NLog.Layouts.LayoutParser.CompileLayout(ConfigurationItemFactory configurationItemFactory, SimpleStringReader sr, Boolean isNested, String& text)
   at NLog.Layouts.SimpleLayout.set_Text(String value)
   at NLog.Layouts.Layout.op_Implicit(String text)
   at NLog.Web.Tests.LayoutRenderers.AspNetSessionValueLayoutRendererTests.SessionWithCulture()

the packages.config shows NLog 4.0.1.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks, I will try to reproduce it locally first.

You have tested it with NLog 4.0.1? Which Visual Studio and Resharper?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested it locally VS2013 update 5+R# 10.0.2: no problem;

Will try VS2015 also

edit: works also in VS2015

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petemounce please add the following code to the erroneous test case

 InternalLogger.LogFile = @"C:\Temp\nlog.web.log";
 InternalLogger.LogLevel = LogLevel.Trace;

and post the log :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@304NotModified here you go - it's in petemounce/NLog.Web@2ba53a1921cd675f2885e4413c10a08d02cb2433. I branched from up-to-date master, made the code change, and have posted the log into that commit message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petemounce I added some comments there. Not sure if you have seen them. If not, I'm not in a hurry, so NP

@codecov-io
Copy link

Current coverage is 9.58%

Merging #26 into master will not affect coverage as of 9b23b7b

@@            master    #26   diff @@
=====================================
  Files           12     12       
  Stmts          146    146       
  Branches        34     34       
  Methods          0      0       
=====================================
  Hit             14     14       
  Partial          2      2       
  Missed         130    130       

Review entire Coverage Diff as of 9b23b7b


Uncovered Suggestions

  1. +4.11% via ...ringTargetWrapper.cs#194...199
  2. +3.43% via ...ringTargetWrapper.cs#166...170
  3. +3.43% via ...ringTargetWrapper.cs#88...92
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

@304NotModified
Copy link
Member

@304NotModified 304NotModified mentioned this pull request Apr 15, 2016
@304NotModified
Copy link
Member

Hi @petemounce , I think I have fixed the root cause with #52

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.

3 participants