NightOwl888 commented on code in PR #1052:
URL: https://github.com/apache/lucenenet/pull/1052#discussion_r1866072860
##########
src/Lucene.Net.Tests.Replicator/IndexInputStreamTest.cs:
##########
@@ -55,7 +58,10 @@ public void
Read_RemainingIndexInputLargerThanReadCount_ReturnsExpectedSection([
int readBytes = 1.KiloBytes();
byte[] readBuffer = new byte[readBytes];
for (int i = section; i > 0; i--)
- stream.Read(readBuffer, 0, readBytes);
+ {
+ stream.ReadExactly(readBuffer, 0, readBytes); // LUCENENET
specific - calling ReadExactly to ensure we read the exact number of bytes
Review Comment:
In this case, `ReadExactly()` could throw an exception if the stream doesn't
have enough bytes in it, and it will return a less useful message than
`Assert.AreEqual()` would. Please change this test to assert length equality
first before reading the bytes so it is clear why we are getting the failure.
##########
.build/azure-templates/run-tests-on-os.yml:
##########
@@ -105,8 +105,34 @@ steps:
& $installScriptPath -Version $sdkVersion -Architecture $architecture
-InstallDir $installPath
Write-Host "##vso[task.setvariable variable=DOTNET_ROOT_X86;]$installPath"
displayName: 'Use .NET SDK ${{ parameters.dotNetSdkVersion }} (x86)'
+ condition: and(succeeded(), or(contains('${{ parameters.framework }}',
'net8.'), contains('${{ parameters.framework }}', 'net9.')), eq('${{
parameters.vsTestPlatform }}', 'x86'))
Review Comment:
We don't need the `net9.0` x86 bits if the target is `net8.0` x86, so we can
remove `or(contains('${{ parameters.framework }}', 'net8.')` here.
##########
azure-pipelines.yml:
##########
@@ -229,13 +229,22 @@ stages:
displayName: 'Delete temp publish location: $(NuGetArtifactDirectory)'
condition: and(succeeded(), ne(variables['RunPack'], 'false'))
+ - template: '.build/azure-templates/publish-test-binaries.yml'
+ parameters:
+ publishDirectory: $(PublishDirectory)
+ framework: 'net9.0'
+ binaryArtifactName: '$(BinaryArtifactName)'
+ testSettingsFilePath:
'$(Build.ArtifactStagingDirectory)/$(TestSettingsFileName)'
+ configuration: '$(BuildConfiguration)'
+ platform: '$(BuildPlatform)'
+
- template: '.build/azure-templates/publish-test-binaries.yml'
parameters:
publishDirectory: $(PublishDirectory)
framework: 'net8.0'
binaryArtifactName: '$(BinaryArtifactName)'
testSettingsFilePath:
'$(Build.ArtifactStagingDirectory)/$(TestSettingsFileName)'
- configration: '$(BuildConfiguration)'
+ configuration: '$(BuildConfiguration)'
Review Comment:
This is still spelled `configration` in `publish-test-binaries.yml`, so this
value is not being passed through. Please correct `publish-test-binaries.yml`,
as well.
##########
src/Lucene.Net/Support/StreamExtensions.cs:
##########
@@ -0,0 +1,47 @@
+#if !FEATURE_STREAM_READEXACTLY
+using System.IO;
+
+namespace Lucene.Net.Support
+{
+ /*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+ public static class StreamExtensions
Review Comment:
There is no public `Lucene.Net.Support` namespace, so this is not the right
place for this.
If this can be internal, please migrate the extension method to the
`Lucene.Net.Support.IO.StreamExtensions` class.
If this must be public, we should consider moving it to J2N.IO.
IMO, this doesn't need to be public. A consumer will either be on a target
framework that supports `ReadExactly` or one that doesn't. If they are
multi-targeting, then they could consider copying this method into their own
project or suppressing the warning.
##########
src/dotnet/Lucene.Net.Tests.CodeAnalysis/Lucene.Net.Tests.CodeAnalysis.csproj:
##########
@@ -22,14 +22,14 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
- <TargetFramework>net8.0</TargetFramework>
+ <TargetFrameworks>net9.0;net8.0</TargetFrameworks>
Review Comment:
I checked with ChatGPT on this and the advice given was:
> If your analyzers primarily inspect code style or generic syntax,
targeting .NET 8.0 alone is typically adequate.
There is no reason to check this functionality with more than one target
framework, as it runs in MSBuild and we generally don't have any
version-specific code (and it isn't very likely we will). So, let's limit this
to the latest version only, `net9.0`.
##########
azure-pipelines.yml:
##########
@@ -326,6 +335,66 @@ stages:
displayName: 'Test Stage:'
jobs:
+ - job: Test_net9_0_x64
+ condition: and(succeeded(), ne(variables['RunTests'], 'false'))
+ strategy:
+ matrix:
+ Windows:
+ osName: 'Windows'
+ imageName: 'windows-latest'
+ maximumParallelJobs: 8
+ maximumAllowedFailures: 0 # Maximum allowed failures for a
successful build
+ Linux:
+ osName: 'Linux'
+ imageName: 'ubuntu-latest'
+ maximumParallelJobs: 7
+ maximumAllowedFailures: 0 # Maximum allowed failures for a
successful build
+ macOS:
+ osName: 'macOS'
+ imageName: 'macOS-latest'
+ maximumParallelJobs: 7
+ maximumAllowedFailures: 0 # Maximum allowed failures for a
successful build
+ displayName: 'Test net9.0,x64 on'
+ pool:
+ vmImage: $(imageName)
+ steps:
+ - template: '.build/azure-templates/run-tests-on-os.yml'
+ parameters:
+ osName: $(osName)
+ framework: 'net9.0'
+ vsTestPlatform: 'x64'
+ testBinariesArtifactName: '$(TestBinariesArtifactName)'
+ nugetArtifactName: '$(NuGetArtifactName)'
+ testResultsArtifactName: '$(TestResultsArtifactName)'
+ maximumParallelJobs: $(maximumParallelJobs)
+ maximumAllowedFailures: $(maximumAllowedFailures)
+ dotNetSdkVersion: '$(DotNetSDKVersion)'
+
+ - job: Test_net9_0_x86 # Only run Nightly or if explicitly enabled with
RunX86Tests
+ condition: and(succeeded(), ne(variables['RunTests'], 'false'),
or(eq(variables['IsNightly'], 'true'), eq(variables['RunX86Tests'], 'true')))
+ strategy:
+ matrix:
+ Windows:
+ osName: 'Windows'
+ imageName: 'windows-2019'
Review Comment:
Does this need to be `windows-2019`, or can we just use `windows-latest`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
