Run tests on .NET 9, #1019 #1052
Conversation
|
Running tests on ADO now; need to look into GitHub test failures and add LUCENENET comments for ReadExactly usage. Converted to draft. |
…ong conversion overflow, apache#1019
|
This passes on ADO and GitHub fully now; ready for review. See updated PR description for an explanation on the Expressions.JS test change. |
src/dotnet/Lucene.Net.Tests.CodeAnalysis/Lucene.Net.Tests.CodeAnalysis.csproj
Outdated
Show resolved
Hide resolved
paulirwin
changed the title
| @in.Read(inBytes, offset, inBytes.Length - offset); | ||
|
|
||
| var numRead = @in.Read(inBytes, offset, inBytes.Length - offset); // LUCENENET specific - asserting that we read the entire buffer | ||
| assertEquals(inBytes.Length - offset, numRead); |
There was a problem hiding this comment.
We are already doing a length check on line 165, so there is no need to do it again here.
ReadExactly() can potentially throw an exception if the length passed in is larger than the number of bytes left to read. This means that any test that does not assert the length prior to the call to ReadExactly() could potentially throw a less useful exception, which is why I pointed it out in that other example (although, after reviewing again, I see that the length of one side is used to set the length of the other, so the assert would never fail). ReadExactly() could work in this case because the prior length check is being done, thus heading off the exception.
That being said, ReadExactly() doesn't seem all that useful unless there are production cases where the exception would be helpful. In tests, it would be more practical to suppress the warning to keep this in sync with upstream.
There was a problem hiding this comment.
I'd like to please reconsider leaving this as-is as of my latest changes rather than suppressing the warning.
- It adds another assertion that makes our tests more robust than upstream.
- I've already added a LUCENENET-specific comment to it explaining the divergence, as is our standard practice.
- It resolves the warning, in IMO a cleaner way than a suppression would.
- It demonstrates best practices of reviewing the result of the Read call to ensure you didn't hit an end of stream, even if semi-redundantly asserted below.
- It achieves your prior concern about not having an exception being thrown by ReadExactly.
| stream.Read(readBuffer, 0, readBytes); | ||
| { | ||
| var numRead = stream.Read(readBuffer, 0, readBytes); // LUCENENET specific - asserting that we read the entire buffer | ||
| Assert.AreEqual(readBytes, numRead); |
There was a problem hiding this comment.
See my comment in IndexAndTaxonomyRevisionTest.
| @in.Read(inBytes, offset, inBytes.Length - offset); | ||
|
|
||
| var numRead = @in.Read(inBytes, offset, inBytes.Length - offset); // LUCENENET specific - asserting that we read the entire buffer | ||
| assertEquals(inBytes.Length - offset, numRead); |
There was a problem hiding this comment.
See my comment in IndexAndTaxonomyRevisionTest.
| @@ -231,7 +231,9 @@ private void AssertFilesIdentical(FileInfo golden, FileInfo sorted) | |||
| using Stream is2 = sorted.Open(FileMode.Open, FileAccess.Read, FileShare.Delete); | |||
| while ((len = is1.Read(buf1, 0, buf1.Length)) > 0) | |||
| { | |||
| is2.Read(buf2, 0, len); | |||
| var numRead = is2.Read(buf2, 0, len); // LUCENENET specific - asserting that we read the entire buffer | |||
| Assert.AreEqual(len, numRead); | |||
There was a problem hiding this comment.
See my comment in IndexAndTaxonomyRevisionTest.
| @@ -254,7 +256,9 @@ private void AssertFilesIdentical(FileStream golden, FileStream sorted) | |||
| Stream is2 = sorted; | |||
| while ((len = is1.Read(buf1, 0, buf1.Length)) > 0) | |||
| { | |||
| is2.Read(buf2, 0, len); | |||
| var numRead = is2.Read(buf2, 0, len); // LUCENENET specific - asserting that we read the entire buffer | |||
| Assert.AreEqual(len, numRead); | |||
There was a problem hiding this comment.
See my comment in IndexAndTaxonomyRevisionTest.
Run tests on .NET 9
Fixes #1019
Fixes #1037
Description
Adds .NET 9 to the test suite. As part of this, added assertions around Stream.Read calls where the result was not checked.
The behavior of the IL
conv.i8opcode (used in Expressions.JS) has changed in .NET 9 (at least for x64). It now behaves like Arm64. Instead of having to keep up with which matrix of frameworks and architectures produces what is effectively a meaningless value (division by zero then converting to long is undefined behavior in the CLR), this changes the assertion to allow for either value, regardless of framework/architecture.Due to the netstandard1.x warning, this PR also fixes #1037 and suppresses the warning for the code analysis projects on netstandard1.3 that must support Visual Studio 2017.