Put generated InvalidClientHelloDataTruncatedBytes data into one test

[greenEkatherine]

There are 3980 generated test cases in SniHelper_TruncatedData_Fails scenario, it breaks XUnit limits for subresults and therefore doesn't report them properly.
The same change was made in YARP's test dotnet/yarp#1432

[ghost]

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

---

There are 3980 generated test cases in SniHelper_TruncatedData_Fails scenario, it breaks XUnit limits for subresults and therefore doesn't report them properly.
The same change was made in YARP's test dotnet/yarp#1432

Author:	greenEkatherine
Assignees:	-
Labels:	area-System.Net.Security
Milestone:	-

[ManickaP]

So now if one of the test fails we won't know which one and also the rest of them won't get executed.
If we're fine with this, no problem. I'm just making sure all the side-effects are understood and agreed on.

[greenEkatherine]

Thank you for pointing this out. I may add additional info to identify failed test, if it helps - we still have an id.

[stephentoub]

Is this actually coming from xunit itself? Do we see this locally or only in CI? Seems like it's actually coming from AzDo, misspellings and all. Is it causing problems? I'd be inclined to keep it as is and get the local benefits of better debugability, unless there's a real issue.

[stephentoub]

That said, 3980 test cases seems like a lot for this. Are they all valuable? Sometimes theories make it a little too easy to test the same conditions over and over :-)

[greenEkatherine]

Is this actually coming from xunit itself? Do we see this locally or only in CI? Seems like it's actually coming from AzDo, misspellings and all. Is it causing problems? I'd be inclined to keep it as is and get the local benefits of better debugability, unless there's a real issue.

It is only in CI. The possible problem - ignoring test results after the first 1000 cases.

[greenEkatherine]

That said, 3980 test cases seems like a lot for this. Are they all valuable? Sometimes theories make it a little too easy to test the same conditions over and over :-)

That was my question to @wfurt 😃 There are no duplicates, so my suggestion was to stop generating data after we reach broken bytes. It should reduce number of cases.

[stephentoub]

The possible problem - ignoring test results after the first 1000 cases.

Does it:

• not run them in the first place
• ignore only successes
• ignore failures as well

?

We have many large theories throughout runtime. I'd be surprised and worried if this was actually ignoring failures in CI.

[greenEkatherine]

The possible problem - ignoring test results after the first 1000 cases.

Does it:

• not run them in the first place
• ignore only successes
• ignore failures as well

?

We have many large theories throughout runtime. I'd be surprised and worried if this was actually ignoring failures in CI.

I double checked, failures should be counted in final result, the issue is only in reporting for separate cases https://developercommunity.visualstudio.com/t/VSTest-test-publication-miscounts-test-c/909375#T-ND914320

[stephentoub]

Thanks. That thread makes it sound like there isn't actually a problem here, and it's just grouping all cases under the parent?

[greenEkatherine]

Thanks. That thread makes it sound like there isn't actually a problem here, and it's just grouping all cases under the parent?

I think no, it's related to the similar issues where not all test results are published. Parent should show if any test fails, without it I cannot find a confirmation that CI will not ignore failures if they happen after the limit.

[danmoseley]

Nit, as a style thing, we seem (I think?) to most often use the (int, byte[]) syntax (possibly with names) not the old Tuple<X, Y> syntax.

[greenEkatherine]

I found out that it is a publishing issue only, failing test must be indicated correctly in CI. I don't even see warnings in runtime pipeline as it was in YARP pipeline, it's because publishing steps are different. Taking this and feedback into account I would rather decline this change.

[stephentoub]

Taking this and feedback into account I would rather decline this change.

Sounds good. Thanks.

[wfurt]

I'mm sorry for commenting on old and closed issue - but I completely missed it for some reason until @rzikm brought it to my attention.

This was was added by dotnet/corefx#28278 very long time and it feels like simple form of fuzzing e.g. feeding the parser with variances of invalid input and checking that the parsing still fails. It is unlikely IMHO we will ever break it and if so int should be easy to figure out details from local test run.

There are two reasons why I wanted to fix this (since we already made same change for YARP)

• when running tests locally, this kills the scroll buffer so often it is not possible to see other test failures directly.
• It creates thousands of extra entries for each build on each platform. That creates unnecessary burden on CPU and storage. Not huge but we seems to be on constant battle for resources.

I'm wondering if we could reconsider this @stephentoub and take it as small improvement.

[stephentoub]

I'm wondering if we could reconsider this @stephentoub and take it as small improvement.

If we don't believe the tests are valuable, it's fine to get rid of them or simplify them. My pushback was on removing them or changing the structure of them purely to work around the cited limitation, which didn't seem to be correct.

[wfurt]

I think there is some value of feeding in invalid data since SslStream operates on untrusted inputs. But I'm not sure what would be good simplification. I added another invalid data test in #63184 But it seems difficult to come up with somewhat more complete set. For that reason I thought moving this chunk into single test may be useful. We can try to come up with better ways how to fuzz or generate invalid or incomplete data.