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

Adding ActiveIssues for tests failing only on NET Native #917

Merged

Conversation

StephenBonikowsky
Copy link
Member

  • By wrapping the ActiveIssue in an #if #endif based on FEATURE_NETNATIVE we should be able to only skip these test on NET Native and not on NET Core.

@StephenBonikowsky
Copy link
Member Author

This PR needs to be validated on an N from K run.
I'm building my K enlistment and will update here once I have been able to validate the changes.
Anyone who has a recent and valid K build can feel free to try as well.

@roncain
Copy link
Contributor

roncain commented Mar 16, 2016

I'm trying this out with ToF NET Native now

@roncain
Copy link
Contributor

roncain commented Mar 16, 2016

Would you mind adding these new tests to that list? ActiveIssue is #832. They are under Security\TransportSecurity\Tcp\StreamingTests.cs

StreamingTests.NetTcp_TransportSecurity_StreamedRequest_RoundTrips_String
StreamingTests.NetTcp_TransportSecurity_StreamedResponse_RoundTrips_String
StreamingTests.NetTcp_TransportSecurity_Streamed_RoundTrips_String
StreamingTests.NetTcp_TransportSecurity_Streamed_TimeOut_Long_Running_Operation
StreamingTests.NetTcp_TransportSecurity_Streamed_Async_RoundTrips_String
StreamingTests.NetTcp_TransportSecurity_StreamedRequest_Async_RoundTrips_String
StreamingTests.NetTcp_TransportSecurity_StreamedResponse_Async_RoundTrips_String
StreamingTests.NetTcp_TransportSecurity_Streamed_RoundTrips_String_WithSingleThreadedSyncContext
StreamingTests.NetTcp_TransportSecurity_Streamed_Async_RoundTrips_String_WithSingleThreadedSyncContext

@StephenBonikowsky
Copy link
Member Author

I'll add those as well Ron, I can confirm that it works as expected.
I am currently checking on some additional failures I am getting to see if they are legitimate or not.

@StephenBonikowsky StephenBonikowsky force-pushed the ActiveIssueForNetNative branch from 44d0ab3 to 2fcf168 Compare March 16, 2016 23:50
@StephenBonikowsky
Copy link
Member Author

Verified all of this works on NET Native.
Had to add the #if !FEATURE_NETNATIVE because otherwise the tests would still run and then fail, test logic only seems to pay attention to the first ActiveIssue.

[ActiveIssue(544, PlatformID.AnyUnix)]
#if FEATURE_NETNATIVE
[ActiveIssue(544)] // Server certificate validation not supported in NET Native
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Do we expect this test will pass on *nix now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, @mconnew would know. The issue implies that it works on Windows but not Unix, there is a later comment that says it is now supported on UWP.
If Matt fixed it and it now works on Unix he should verify it and remove the ActiveIssue?

@roncain
Copy link
Contributor

roncain commented Mar 17, 2016

LGTM.
I also verified on NET Native, and it looks good. I can't verify on Linux because we are currently broken running on Linux due to System.Private.Uri. This should be fixed in CoreFx via https://github.com/dotnet/corefx/blob/be08afd9701053f5f9369762db58b87c8ca9559f/src/System.Private.Uri/pkg/unix/System.Private.Uri.pkgproj#L11 but we need to wait for new packages to be build. Don't block the merge based on this.

* By wrapping the ActiveIssue in an #if #endif based on FEATURE_NETNATIVE we should be able to only skip these test on NET Native and not on NET Core.
@StephenBonikowsky StephenBonikowsky force-pushed the ActiveIssueForNetNative branch from 2fcf168 to b84caec Compare March 17, 2016 16:50
StephenBonikowsky added a commit that referenced this pull request Mar 17, 2016
Adding ActiveIssues for tests failing only on NET Native
@StephenBonikowsky StephenBonikowsky merged commit fcb863e into dotnet:master Mar 17, 2016
@StephenBonikowsky StephenBonikowsky deleted the ActiveIssueForNetNative branch March 17, 2016 17:03
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.

5 participants