-
Notifications
You must be signed in to change notification settings - Fork 67
Adds netstandard2.0 target for dependencycollector. #1234
Conversation
…ed to target 2.1 instead of 1.1. Unittests were blind about 2.0 before this, and now they will be blind about 1.1 instead.
public async Task TestDependencyCollectionDnsIssue() | ||
{ | ||
using (var module = new DependencyTrackingTelemetryModule()) | ||
{ | ||
module.Initialize(this.config); | ||
|
||
var request = new HttpRequestMessage(HttpMethod.Get, $"http://{Guid.NewGuid()}"); | ||
var url = $"http://{Guid.NewGuid()}:5050"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is port 5050 ?
would this test still work if we used a non-routable IP address? (ex: 10.0.0.0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5050 was just random!
The goal of this test is to simulate a clientside exception - the easiest way to trigger that is to fail in DNS itself. Its not easy to simulate any other network situation in unit tests.
I understand the reason for switching from 1.1 to 2.1, but this makes me just a little bit nervous. Other than that, this looks good to me. :) |
@MS-TimothyMothra we were running without 2.0 tests until now :( Since 1.1 is deprecated officially, we can live without testing 1.1. Anyway if all insist for 1.1 tests, I am happy to get it added. |
Plan is to anyway cut support for 1.1 by the time 3.0 releases. So it is okay to shift unit tests to target 2.1 instead of 1.1. |
UnitTests converted to target 2.1 instead of 1.1. Unit tests were blind about 2.0 before this, and now they will be blind about 1.1 instead. .NET Core 1.1 is already officially out of support, but SDK still has target for 1.1
We may chose to have 1.1 and 2.1 tests, instead of 2.1 alone.
Fix Issue #1212 .
For significant contributions please make sure you have completed the following items:
Changes in public surface reviewed
Design discussion issue #
CHANGELOG.md updated with one line description of the fix, and a link to the original issue.
The PR will trigger build, unit tests, and functional tests automatically. If your PR was submitted from fork - mention one of committers to initiate the build for you.
If you want to to re-run the build/tests, the easiest way is to simply Close and Re-Open this same PR. (Just click 'close pull request' followed by 'open pull request' buttons at the bottom of the PR)
Please follow [these] (https://github.com/Microsoft/ApplicationInsights-dotnet-server/blob/develop/CONTRIBUTING.md) instructions to build and test locally.