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

Test review Core I-J #926

Merged
merged 76 commits into from
Mar 21, 2024
Merged

Test review Core I-J #926

merged 76 commits into from
Mar 21, 2024

Conversation

paulirwin
Copy link
Contributor

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Partial work for #259 for the core I-J tests assembly.

Sorry, something went wrong.

@paulirwin paulirwin marked this pull request as ready for review March 11, 2024 23:37
@paulirwin paulirwin requested a review from NightOwl888 March 11, 2024 23:37
Copy link
Contributor

@NightOwl888 NightOwl888 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

There are a few requested changes as well as some other feedback.

src/Lucene.Net.Tests/Index/TestIndexWriterConfig.cs Outdated Show resolved Hide resolved
src/Lucene.Net.Tests/Index/TestIndexWriterExceptions.cs Outdated Show resolved Hide resolved
src/Lucene.Net.Tests/Index/TestIndexWriterReader.cs Outdated Show resolved Hide resolved
src/Lucene.Net.Tests/Index/TestNoMergePolicy.cs Outdated Show resolved Hide resolved
src/Lucene.Net.Tests/Index/TestPayloads.cs Outdated Show resolved Hide resolved
src/Lucene.Net.Tests/Index/TestStressNRT.cs Outdated Show resolved Hide resolved
src/Lucene.Net.Tests/Index/TestTermsEnum.cs Outdated Show resolved Hide resolved
@NightOwl888
Copy link
Contributor

It looks like there were a couple of omissions.

  1. This line will need to be reenabled to prevent the NONE option from being passed.
  2. This line will need to be fixed to exclude the NONE option from being tested. Here is how we do it elsewhere.

I am still torn on whether to just hide the NONE option from users because it really isn't a thing anywhere but in the FindMerges() method and it isn't very likely users would enable asserts to find out that they are passing something that isn't valid to calls to other methods that accept MergeTrigger. We could add guard clauses to all of those methods to make it fail without enabling asserts, but it is much simpler just to make it invisible.

- add LUCENENET to C#-specific comment
- add back unused using for Assert as safeguard with comment
- use invariant culture for number conversion in
  TestIndexWriterExceptions
@paulirwin paulirwin requested a review from NightOwl888 March 13, 2024 23:32
@paulirwin paulirwin merged commit 444e6d0 into apache:master Mar 21, 2024
199 checks passed
@paulirwin paulirwin deleted the test-review-i-j branch March 21, 2024 15:22
@NightOwl888
Copy link
Contributor

I setup a new repository with our Azure DevOps templates and discovered that there are some breaking changes.

Breaking Change 1

There is a breaking change introduced by Microsoft in the .NET 8 SDK, which appends the Git commit hash to the AssemblyInformationalVersion: dotnet/sdk#34568. We already add a shortened Git commit hash, so this will add it twice. The fix should definitely be in this PR, since this is where we introduce building with the .NET 8 SDK.

More info about it is here: https://learn.microsoft.com/en-us/dotnet/core/compatibility/sdk/8.0/source-link. I have tested the solution and it seems to meet our needs. However, the first build after I changed it caused the NuGet packages to be built without any DLLs in them, but the second build succeeded. Not sure whether that was a fluke or a problem.

We can put the fix in the default Directory.Build.targets file of the solution (in each repository):

  <PropertyGroup Label="Versioning">
    <GenerateAssemblyVersionInfo>false</GenerateAssemblyVersionInfo>
  </PropertyGroup>

Breaking Change 2

The default for a new pipeline in Azure DevOps is to create a shallow clone. Previously, it would create a deep clone by default.

This doesn't break anything on Lucene.Net, but all of the other repositories that we own use NerdBank.GitVersioning, and this is a problem for generating the correct version number. There is an issue about it here: dotnet/Nerdbank.GitVersioning#837 and the fix is here: https://github.com/dotnet/Nerdbank.GitVersioning/blob/main/doc/cloudbuild.md#azure-pipelines

We would need to apply that on the "Build" job of all of our repositories. On Lucene.NET, it is already done this way for performance reasons.

Our Other Repositories

This is a list of all of the repositories that use the same Azure DevOps (and command line build) templates that will need to be patched to account for this.

While we could just fix the issue in this PR and add 2 issues to each of the other repositories, I think it might be simpler just to submit a PR to each repository with the patches and a link back to this PR.

@paulirwin
Copy link
Contributor Author

@NightOwl888 Thanks for the info and research! I think your comment was intended for PR #930 though?

@NightOwl888
Copy link
Contributor

@NightOwl888 Thanks for the info and research! I think your comment was intended for PR #930 though?

@paulirwin - You are absolutely right. I goofed. I will re-post over there. Also, I realized I missed one.

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.

None yet

2 participants