-
Notifications
You must be signed in to change notification settings - Fork 561
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
Rename test files with release version #1366
Rename test files with release version #1366
Conversation
LGTM, ran locally, looks good |
When I run "N on K using RTM packages" with this PR using our prior instructions, I get failures due to missing API's. I think we need to check with @shmao on the proper way to test this againt "N on K using RTM packages" now. It might be that we need to update the CLRTestContractReferences in the ToF wrapper projects to later versions now. One big one is the absence of X509Certificate2.Thumbprint in RTM. Fortunately, recent infrastructure changes tolerate the MissingMethodException in the ConditionalFact detectors and just cause the cert ConditionalFacts to all return false. But there are some tests that explicitly reference Thumbprint and fail -- so those might need to be reverted to "pre-release" naming, depending what we discover about CLRTestContractReference It might be that the PR is fine and we only need to fix the ToF side, but it will take some experimentation. Examples of missing-API failures:
|
Hi Ron, sorry I should have mentioned in my PR that I still had to do the run you just did in order to find additional tests that need to be moved out of the newly renamed files into “current” pre-release test files. After discussing this with Zhenlan we don’t care about RTM, just most recently released version. So yes we will probably have to make a few changes, I will follow-up with Shin. I also expected a few tests would need to be moved to the “current” pre-release test files. I got side-tracked yesterday by the TFS build break and didn’t get to this. -Stephen From: Ron Cain [mailto:notifications@github.com] When I run "N on K using RTM packages" with this PR using our prior instructions, I get failures due to missing API's. I think we need to check with @shmaohttps://github.com/shmao on the proper way to test this againt "N on K using RTM packages" now. It might be that we need to update the CLRTestContractReferences in the ToF wrapper projects to later versions now. One big one is the absence of X509Certificate2.Thumbprint in RTM. Fortunately, recent infrastructure changes tolerate the MissingMethodException in the ConditionalFact detectors and just cause the cert ConditionalFacts to all return false. But there are some tests that explicitly reference Thumbprint and fail -- so those might need to be reverted to "pre-release" naming, depending what we discover about CLRTestContractReference It might be that the PR is fine and we only need to fix the ToF side, but it will take some experimentation. Examples of missing-API failures: Running Test: HttpsTests.CrossBinding_Soap11_EchoString Caught Unexpected exception:System.Exception: Method 'OSPlatform.get_Windows()' was not included in compilation, but was referenced in ConditionalTestDetectors.IsWindows(). There may have been a missing assembly.
b__0_7() in E:\ProjectK\src\QA\ToF\PN\x86\rel\IL\FX\Conformance\System.Private.ServiceModel\4.0.0.0\WcfOpen\Scenarios\Security\TransportSecurity\Security.TransportSecurity.Tests.main.cs:line 20
---- Test FAILED --------------- Running Test: HttpsTests.ClientCertificate_EchoString Caught Unexpected exception:System.Exception: Method 'BasicHttpsBinding..ctor(BasicHttpsSecurityMode)' was not included in compilation, but was referenced in HttpsTests.ClientCertificate_EchoString(). There may have been a missing assembly.
b__0_12()
---- Test FAILED --------------- Running Test: Http_ClientCredentialTypeTests.DigestAuthentication_Echo_RoundTrips_String_No_Domain Caught Unexpected exception:System.ServiceModel.CommunicationException: net_http_client_execution_error ---> System.Net.Http.HttpRequestException: net_http_client_execution_error ---> System.NotSupportedException: NotSupported_CloningNotSupported, GetInputStreamAt
Running Test: Tcp_ClientCredentialTypeCertificateCanonicalNameTests.Certificate_With_CanonicalName_Localhost_Address_EchoString Caught Unexpected exception:System.Exception: Method 'ClientCredentials.get_ServiceCertificate()' was not included in compilation, but was referenced in Tcp_ClientCredentialTypeCertificateCanonicalNameTests.Certificate_With_CanonicalName_Localhost_Address_EchoString(). There may have been a missing assembly.
b__0_26()
---- Test FAILED --------------- Running Test: Tcp_ClientCredentialTypeTests.TcpClientCredentialType_Certificate_EchoString Caught Unexpected exception:System.Exception: Method 'X509Certificate2.get_Thumbprint()' was not included in compilation, but was referenced in Tcp_ClientCredentialTypeTests.TcpClientCredentialType_Certificate_EchoString(). There may have been a missing assembly.
b__0_29()
---- Test FAILED --------------- — |
Just checking @StephenBonikowsky -- renaming the PeerTrust tests back to their original names -- does that make those tests exactly as they were prior to this PR (I know the files themselves will be different)? I've been working on those tests this week and will need to merge after you checkin. To save merge conflicts, I think I should wait for your PR to go first. P.S. I'm happy to run N-on-K-latest-packages tests in Tof on this PR when you tell me it's ready. |
I'm not changing the names of any tests, or is that what you are doing? This PR makes no change in terms of the namespace and name of any tests. I've been running the N-on-K starting yesterday, it is more complicated than just running against latest packages, @shmao put something together for me yesterday but I still have very bad results, need to work with him on this today. |
@StephenBonikowsky -- I was just verifying that after you merge, the tests will still be in the files where they are now (pre-merge), but the file itself will be different since the other tests have been moved. No biggie -- I've been adding new PeerTrust tests and renaming older ones. I'll just merge carefully. BTW, N-on-K seems broken right now, so I don't think either of us can verify it works 😦 |
Well Shin and I are trying, we are using published builds which should be ok. From: Ron Cain [mailto:notifications@github.com] @StephenBonikowskyhttps://github.com/StephenBonikowsky -- I was just verifying that after you merge, the tests will still be in the files where they are now (pre-merge), but the file itself will be different since the other tests have been moved. No biggie -- I've been adding new PeerTrust tests and renaming older ones. I'll just merge carefully. BTW, N-on-K seems broken right now, so I don't think either of us can verify it works 😦 — |
I got the N on K run working using ILC version 24308.00 from a few days ago, this was good enough to validate what has changed since the netcore50 release. |
c713724
to
6a7c493
Compare
Missed one file, amended the previous commit. |
… S.P.SM - Appended 4.1.0 to the names of the test files.
* These tests can't work in released S.P.SM version 4.1.0 as needed product code has only been added since then, moving them into a new pre-release test file.
* ServiceIdentityNotMatch_Throw_MessageSecurityException and TCP_ServiceCertFailedCustomValidate_Throw_Exception were both failing on netnative because the expected exception was different. This was fixed in PR dotnet#1337 therefore these tests need to be moved into current pre-release test files. * Tests under Scenarios/Contract/Message and Scenarios/Extensibility/MessageInterceptor needed supporting common code files located in their same directory to also be renamed so these files would also be picked up by the test run logic used to validate these tests against the last released libraries using the latest tool-chain.
6a7c493
to
229d11a
Compare
Updated once more, it turns out there is a product bug in netnative being hit by the MessageInterceptor test, I will open an Issue for it but this test needs to remain in a pre-release test file. |
Opened Issue #1374 Did not add it as an active issue to the test case yet as I would like an independent confirmation. |
LGTM subject to you having run N on K. Thanks for dealing with this :) |
@StephenBonikowsky -- I can't repro the issue reported for #1374 using normal N-on-K. But I would predict exactly the failure reported in that issue if it were tested using latest WCF shipped packages. |
In the renaming I did not include the scenario test file for infrastructure as I don't think this applies.
I moved PeerTrust tests into new files but after an initial pass I didn't find other tests that have had product fixes since 4.1.0 release for Windows, only *nix.