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

Add unit tests for Collections, #1116 #1124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

Add unit tests for Collections, including some Harmony tests.

Fixes #1116

Description

This PR adds unit tests for the Lucene.Net.Support.Collections type. Some of the tests are from Harmony where applicable, and others are lucenenet-specific.

Similar to the discussion at #1121 (comment), this fixes some culture/locale inconsistencies with Java. It also removes some ToString methods that were Culture-aware overloads, but these overloads were unused (and this is an internal class, so that's not a breaking change).

As noted in the methods, Collections.ToString is based on the toString method from Java's AbstractCollection/AbstractMap. In Java, this method is not Locale-aware, so that the output is consistent across locales. As such, it does not format numbers using i.e. a decimal comma, nor does it use a Locale-aware list separator. It is neither good for serialization/deserialization, nor for display in a UI. It is simply a string representation to be used for debugging and diagnostic purposes and is intended to be closer to a code representation than user-facing. (Even then it's arguably poor in some ways, as strings are not enclosed in double quotes, but it is what it is.)

For example, this Java code (can be run with JBang if you'd like) sets the current Locale to fr-FR, which has a decimal comma, but it does not use that in the output. (Java does not have a list separator concept in its Locales, as far as I know, but if it did, that should use a semicolon for fr-FR if it were Locale-aware.)

///usr/bin/env jbang "$0" "$@" ; exit $?

import java.text.*;
import java.util.*;
import static java.lang.System.*;

public class hello {

    public static void main(String... args) {
        Locale.setDefault(new Locale("fr", "FR"));
        List<Object> list = new ArrayList<>();
        list.add(list);
        list.add(1);
        list.add('a');
        list.add(2.1);
        list.add("xyz");
        list.add(new ArrayList<>() { { add(1); add(2); add(3); } });
        list.add(null);
        System.out.println(list); // prints: [(this Collection), 1, a, 2.1, xyz, [1, 2, 3], null]
        // to demonstrate that the locale is applying when applicable...
        NumberFormat nf = NumberFormat.getInstance();
        System.out.println(nf.format(2.1)); // prints: 2,1
    }
}

Note the comma list separator and decimal point that indicates that the output is not Locale-aware. I set up the List-based test to exactly mimic this behavior, and assert the same output. This discovered that the test failed for cultures with a decimal comma, since it was just calling .ToString() on the value, which will use the current thread's culture. I changed this to use Convert.ToString with passing in the invariant culture to match Java's behavior. These ToString methods are only used in building exception messages or console app outputs so that fits the bill of diagnostic purposes that do not need to be locale-aware.

Additionally, it was discovered that the Object-based ToString method could fail if the argument was null, so this has been fixed along with making the file enabled for nullable type checking. This caused some inconsistencies in warnings between .NET Framework, .NET Standard, and .NET 8, so if it seems like the annotations are a little too much, this was likely because of getting warnings in one of the other targets. In particular, the private IComparer implementations were made aggressively nullable.

@paulirwin paulirwin added the notes:improvement An enhancement to an existing feature label Jan 27, 2025
@paulirwin paulirwin requested a review from NightOwl888 January 27, 2025 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes:improvement An enhancement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Support unit tests for Collections
1 participant