search

apache / lucenenet

Apache Lucene.NET
https://lucenenet.apache.org/
Apache License 2.0
2.24k stars 638 forks source link

Review tests for Lucene.Net assembly #259

Open clambertus opened 5 years ago

clambertus commented 5 years ago

Occasionally, tests are still being found with important lines that have been commented because the functionality didn't exist at the time they were ported. Additionally, some test conditions have been changed from what they were in Java (although in some cases this is a necessary change due to a difference in platforms and in other cases it is a bug). We need some assurance that none of our green tests are false positives.

A line-by-line review would be best, but at the very least we should be checking that the implementations are complete and the test conditions are the same as in Java Lucene 4.8.0. Checking at the method level to ensure they all exist and have the right attributes has been completed already on Lucene.Net (core).

The tests in question need to be analyzed to ensure:

  1. That the test conditions weren't changed from what they were in Lucene (without some very good reason)
  2. That none of the asserts or parts of the test setup were simply commented out to make the test pass rather than fixing the underlying problem
  3. (for tests other than Lucene.Net (core)): The test methods/classes are not skipped in Lucene.NET unless they were skipped in Lucene or have some known problem

There may be other unforeseen issues with the tests (such as an incorrect translation of the line), as well, which is why a line-by-line comparison would be best.

Some Other Things to Look For

  1. Missing [Test] attributes. Note that in Lucene tests are run by naming convention for any parameterless method named starting with test, but in .NET the attribute is required.
  2. Formatting of arrays over multiple lines
  3. Stripped comments which would provide needed context
  4. Partially implemented classes/tests
  5. Method calls such as Convert.ToString(int) that require an explicit CultureInfo.Invariant parameter to match Java
  6. APIs that accept/return object that are using value types (boxing/unboxing)
  7. APIs that require casting in order to use them, which is something we should strive to solve
  8. APIs that accept/return non-generic collections, which we should strive to make generic

While it isn't the most important part of the task, the tests are also the easiest place to spot usability issues with the API, so if any are discovered (that aren't already marked LUCENENET TODO) we should open new issues for them as well.

The test projects that need review are:

JIRA link - [LUCENENET-632] created by nightowl888

rclabo commented 3 years ago

The first paragraph and the 2nd paragraph don't seem quite congruent. Is the primary issue here that some lines of the tests may be commented out because the functionality didn't exist at the time the test was ported. If so, then I suggest the course of action is to visually inspect all tests for the projects listed to ensure no needed lines of code are commented out. If any commented lines are found then uncomment them and run the test to ensure it still passes. If that approach is sufficient for this issue then I'm willing to do that work and this issue can be assigned to me.

NightOwl888 commented 3 years ago

@rclabo

I have updated the text of the original task to make it clearer. I have also broken down the task by test project. If necessary, we can further break it down by namespace to divide up the work between multiple people.

Let me know if you are still on board with doing (at least some of) this task.

rclabo commented 3 years ago

Let me do a dry run on some of the tests in Lucene.Net.Tests._A-D to get a sense of the time requirement for this. Lucene.Net has 7735 tests, which is no small amount. I need some sense of the time it's gonna take before I can know what I can sign up for. I'll report back here after I review some and see what level of effort is required.

NightOwl888 commented 3 years ago

I will begin working on the Lucene.Net.Tests.Facet tests.

NightOwl888 commented 3 years ago

@rclabo

During #411 I found 2 API issues and 4 bugs (places where the code diverged from Java or were converted incorrectly to .NET). I also timed my progress and it took an average of about 3 minutes per test to do a line-by-line review. For someone less familiar with the project, that number can probably be adjusted to 3.5-4 minutes per test for an estimate of the remaining projects.

rclabo commented 3 years ago

@NightOwl888 thanks, that's excellent info. I think I will learn in the test reviewing process. I see that you can check off the test set you reviewed in the list contained in this issue. That's cool.

NightOwl888 commented 3 years ago

I will begin working on the Lucene.Net.Tests.Join tests.

NightOwl888 commented 3 years ago

I will begin working on the Lucene.Net.Tests.Queries tests.

rclabo commented 3 years ago

I'll review Lucene.Net.Tests.Expressions

rclabo commented 3 years ago

I'll work on Lucene.Net.Tests._A-D

NightOwl888 commented 3 years ago

@rclabo

That's great. However, IMO it might make sense to hold off on reviewing Lucene.Net tests until after we have converted the exceptions and catch blocks in #446.

If you have already started, just continue to the end of the tests in that assembly, but we certainly should wait on the index reader/writer tests until after this is done. There may also be issues that are resolved by #446 in codecs.

NightOwl888 commented 3 years ago

@rclabo - #446 has been merged, so we can resume reviewing Lucene.Net (core) tests.

rclabo commented 3 years ago

Thanks for the nudge. Also THANK YOU for the huge work you did in reviewing all the exceptions and the catch clauses to make sure they are faithfully following the behavior of Java Lucene. That was a huge amount of super important work.

paulirwin commented 10 months ago

I have begun reviewing the core T-Z tests. So far I've made it through Automaton and it was all good apart from some very minor formatting that I'll PR once complete.

Update 1/12: Reviewed Util.Fst, also mostly a few minor formatting changes Update 1/12: Reviewed Util.Packed, same Update 1/15: Reviewed files with names starting with B-TestF in Util Update 1/17: Reviewed through TestQ in Util Update 1/18: Finished last segment of tests, PR coming soon

rclabo commented 10 months ago

@paulirwin Thank you Paul, much appreciated! This is such a great project, so I get excited when I see others jumping in to help. Thank you!

paulirwin commented 10 months ago

@rclabo haha I'm jumping in again after a very long absence. My colleagues and I had done a lot of work to finish getting the core of 4.8 ported about 10 years ago, then another group came along with a branch and that ended up being the current work we see today. We used my branch to create an app that supported ~1B document indexes under heavy load in production, so it was pretty rock solid at the time, but was stuck on .NET Framework so I'm happy to see the state of things today and being able to build this on arm64 macOS in .NET 6! I haven't had a need for Lucene.net in a while so I have been pretty quiet the last several years. Now, though, I have a project that is a local emulator for Azure Search which uses 4.8 beta, as well as I'm getting into RAG with LLMs so I'd love to see if I can help move the project along to be able to use the new vector database fields in modern Java Lucene.

The tests we ported at the time can be referenced if desired: https://github.com/paulirwin/lucene.net/tree/branch_4x

rclabo commented 10 months ago

@paulirwin Super cool. I'm also very interested in LLMs and RAGs. Well, welcome back. Lucene and the community have missed you! :-)

NightOwl888 commented 10 months ago

@paulirwin - Thanks for your help on this.

This issue is certainly a good place to start after being inactive for awhile. But do note we have set up a Slack channel #lucenenet-dev where you can review some (more) of the recent activity or discuss other aspects of the project that are off-topic for a specific GitHub issue. The dev mailing list works, also, but I find it is easier to share code on Slack.

Apache docs for Slack

paulirwin commented 9 months ago

T-Z done and merged, moving backwards to the J-S monster (1,158 tests):