Skip to content
This repository was archived by the owner on Dec 19, 2018. It is now read-only.

Porting "Always log startup exceptions to 1.1.4" #1184

Closed
wants to merge 1 commit into from

Conversation

jkotalik
Copy link
Contributor

Fix for #1160
Had to modify tests to work with 1.1.x

@dnfclas
Copy link

dnfclas commented Aug 24, 2017

@jkotalik,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@muratg
Copy link

muratg commented Aug 24, 2017

Why is the branch 1.1.4? Shouldn't it be 1.1.3?

cc @pranavkm

@jkotalik jkotalik force-pushed the jkotalik/patch/1.1.4 branch from 1368d1c to 3f7cb64 Compare August 24, 2017 22:20
var builder = CreateWebHostBuilder()
.CaptureStartupErrors(false)
.ConfigureServices(collection => collection.AddSingleton<ILoggerFactory>(factory))

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove empty line


Assert.Throws<InvalidOperationException>(() => builder.Build());

Assert.True(testSink.Writes.Any(w => w.Exception?.Message == "Startup exception"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Assert.Contains("Startup exception", testSink.Writes.Select(w => w.Exception?.Message).Where(m => m != null)); is slightly better because it'll cause xunit to print the 2nd argument (the haystack). Helps when the test fails. With the current approach all you get is a Assert.True(false) which is less useful.

@pranavkm
Copy link
Contributor

@muratg yup, the branch needs to rel/1.1.3. We use patch/* branches to make infrastructural \ test \ non-src changes to produces (for .e.g flaky tests), but release branches always follow the package version.

@jkotalik you'll have to

a) Create a rel/1.1.3 branch (git checkout patch/1.1.4 -b rel/1.1.3
b) Bump up the version of Hosting to 1.1.3 and update all dependencies (see @JunTaoLuo's email)

@JunTaoLuo
Copy link
Contributor

JunTaoLuo commented Aug 24, 2017

FYI, don't bump the version of Hosting to 1.1.3 in this PR, use Coherence-Patch according to the instruction in the email instead.

@jkotalik
Copy link
Contributor Author

Closing to merge with rel/1.1.3

@muratg
Copy link

muratg commented Aug 25, 2017

Thanks folks! We should ensure the other patch check-ins follow these guidelines as well.

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

Successfully merging this pull request may close these issues.

5 participants