Test review E-I, #259

[paulirwin]

• 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.

Test review for core E-I tests

Partial #259

Description

This updates the tests in the E-I test assembly to match upstream Lucene for style, formatting, etc. Additionally, some minor bugs (including documentation bugs) have been fixed.

[paulirwin]

@NightOwl888 Updated title to indicate the breaking changes of changing some static readonly fields to be const. This will affect any NuGet packages or custom binaries that use these fields, although it's generally not breaking from a source standpoint. We should probably do a sweep before beta 18 for other const-able fields.

[NightOwl888]

@NightOwl888 Updated title to indicate the breaking changes of changing some static readonly fields to be const. This will affect any NuGet packages or custom binaries that use these fields, although it's generally not breaking from a source standpoint. We should probably do a sweep before beta 18 for other const-able fields.

Looks like this only affects test classes except for the Assert class that is part of the test framework, but there are no breaking API or behavioral changes in it. Or am I missing something?

[paulirwin]

IndexWriter and ByteBlockPool have some public const changes. I can revert those if we'd rather do them as part of a separate sweep.

[NightOwl888]

IndexWriter and ByteBlockPool have some public const changes. I can revert those if we'd rather do them as part of a separate sweep.

I see 'em. I have 7 files left to review and those 2 are at the bottom.

[NightOwl888]

If these are made const, that means they will be compiled as metadata in the consuming assembly. So any changes to the values will not be seen unless the consuming app is recompiled.

This may or may not be a problem, though. Technically, we should never update the values on these unless porting a new version of Lucene. It would be helpful to know if Lucene ever changes values during minor or patch version bumps, though, because we will eventually catch up with them. The safest bet without knowing that is to leave them static. That said, I have been guilty of changing many of these to const recently.

[NightOwl888]

Nice work!

I found a few minor things to address. Please see my comments inline.

[paulirwin]

@NightOwl888 All feedback items should be addressed now. Thanks!