Skip to content
This repository has been archived by the owner on Jul 5, 2020. It is now read-only.

Adds netstandard2.0 target for dependencycollector. #1234

Merged
merged 4 commits into from
Jul 16, 2019

Conversation

cijothomas
Copy link
Contributor

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 .

  • I ran Unit Tests locally.

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.

…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.
@cijothomas cijothomas added this to the 2.11 milestone Jul 15, 2019
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";
Copy link
Member

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)

Copy link
Contributor Author

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.

@TimothyMothra
Copy link
Member

I understand the reason for switching from 1.1 to 2.1, but this makes me just a little bit nervous.
I would feel better if we added 2.1 now, and removed 1.1 at a later date.

Other than that, this looks good to me. :)

@cijothomas
Copy link
Contributor Author

@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.
(I am planning to soon add 3.0 tests, so want to get rid of 1.1 sooner...)

@cijothomas cijothomas closed this Jul 15, 2019
@cijothomas cijothomas reopened this Jul 15, 2019
@cijothomas
Copy link
Contributor Author

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.

@cijothomas cijothomas merged commit a6492f6 into develop Jul 16, 2019
@cijothomas cijothomas deleted the cithomas/dependency20target branch July 16, 2019 16:00
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.

2 participants