Consider instance name in TreeReference comparison

[lognaturel]

Closes #449

Initial description

I see two testing options: * create `TreeReference`s and verify the result of testing for equality between them * add a simple form that exhibits this problem

I don't like the first because we don't have tests of that kind. I could target just the new behavior which would lead to trivial tests. Or I could try to fully test the equals method which is likely to be a bit of a mess with all the multiplicity and predicate options. Admittedly, maybe the latter is a reason to write those tests but I ran into this by accident and don't want to rathole here.

I don't love the second because I don't think that form would really augment the test suite in a meaningful way.

That said, I can be convinced that I have a bad attitude and should Do The Right Thing™️. (CC @seadowg)

What has been done to verify that this works as intended?

Tried form from issue in Collect.

Why is this the best possible solution? Were any other approaches considered?

This is the simplest possible way to compare two objects that can be null.

What has been done to verify that this works as intended?

Tried form from issue in Collect and added a test (thank you @ggalmazor and @seadowg).

Why is this the best possible solution? Were any other approaches considered?

I would have liked to use Java's Objects.equals but that's not available in Android API levels < 19. See getodk/collect#3192.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

The intent is for this to have no user impact other than enabling previously impossible behavior. If I messed this up and equality of TreeReferences has changed in an unexpected way, it could lead to problems with any kind of form, though.

Do we need any specific form for testing your changes? If so, please attach one.

dont-show-options-selected-in-other-instances.xml.txt

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

[codecov-io]

Codecov Report

Merging #450 into master will decrease coverage by 0.21%.
The diff coverage is 75%.

@@             Coverage Diff              @@
##             master     #450      +/-   ##
============================================
- Coverage     48.91%   48.69%   -0.22%     
+ Complexity     2930     2913      -17     
============================================
  Files           245      242       -3     
  Lines         13683    13627      -56     
  Branches       2645     2642       -3     
============================================
- Hits           6693     6636      -57     
- Misses         6146     6149       +3     
+ Partials        844      842       -2

Impacted Files	Coverage Δ	Complexity Δ
...rg/javarosa/core/model/instance/TreeReference.java	67.16% <100%> (+0.24%)	89 <0> (+1)	⬆️
src/main/java/org/javarosa/core/util/Objects.java	50% <50%> (ø)	4 <4> (?)
...n/java/org/javarosa/core/model/actions/Action.java	57.14% <0%> (-29.53%)	5% <0%> (-3%)
...a/org/javarosa/core/services/PrototypeManager.java	79.16% <0%> (-8.34%)	8% <0%> (-1%)
src/main/java/org/javarosa/core/model/FormDef.java	60.79% <0%> (-0.21%)	121% <0%> (-1%)
...ain/java/org/javarosa/xform/parse/XFormParser.java	64.63% <0%> (-0.07%)	235% <0%> (ø)
.../java/org/javarosa/core/model/CoreModelModule.java	16.66% <0%> (ø)	1% <0%> (ø)	⬇️
...del/actions/setlocation/StubSetLocationAction.java
.../actions/setlocation/SetLocationActionHandler.java
...e/model/actions/setlocation/SetLocationAction.java
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6120647...3b19242. Read the comment docs.

[ggalmazor]

The change is looking good, but I'd like to ask for some tests ;)

From the two options you have considered, I'd suggest doing at least option 1. It looks like doing option 2 too won't provide much value, but I'll leave it to you ;)

Option 1 is quick to do and provides a sanity check. You can even take the chance to use a good test method name and throw some comments to document the change and the whys.

You could also document in the test methods the consequences that this new equality criterion would have in a form. I think you've started to list that in your PR description.

[seadowg]

Yeah I think I agree with @ggalmazor that going with option 1 seems like the way to go.

[lognaturel]

🙏 Thank you. Please let me know if you see a better approach or additional tests I should add.

[ggalmazor]

The test is a great example of a good place to use the junit Parameterized runner class:

@RunWith(Parameterized.class)
public class TreeReferenceEqualsTest {
    @Parameterized.Parameter(value = 0)
    public String testCase;

    @Parameterized.Parameter(value = 1)
    public String pathA;

    @Parameterized.Parameter(value = 2)
    public String pathB;

    @Parameterized.Parameter(value = 3)
    public boolean expectToBeEqual;

    @Parameterized.Parameters(name = "{0}")
    public static Collection<Object[]> data() {
        return Arrays.asList(new Object[][]{
            {"same path in primary instance are equal", "/foo/bar", "/foo/bar", true},
            {"same path in secondary instance are equal", "instance('foo')/bar/baz", "instance('foo')/bar/baz", true},
            {"same path in primary and secondary instance are not equal", "instance('foo')/foo/bar", "/foo/bar", false},
            {"same path in different secondary instances are not equal", "instance('foo')/foo/bar", "instance('bar')/foo/bar", false},
            {"different paths in primary instance are not equal", "/foo/bar", "/foo/bar/quux", false},
            {"different paths in secondary instance are not equal", "instance('foo')/foo/bar", "instance('foo')/foo/bar/quux", false}
        });
    }

    // region Equality across same or different instances

    // Prior to JR 1.15, the instance was not taken into account when comparing two TreeReferences. That meant two
    // references representing the same path in two different instances would be considered the same reference.
    @Test
    public void treeReferencesWithSamePathInPrimaryInstance_AreEqual() {
        if (expectToBeEqual)
            assertThat(buildRef(pathA), is(buildRef(pathB)));
        else
            assertThat(buildRef(pathA), is(not(buildRef(pathB))));
    }

    //endregion
}

[seadowg]

@ggalmazor for some reason Github is not letting my react to that post so here is my reaction: 👍

[lognaturel]

Good point, @ggalmazor, thank you. I used the parameterized approach with a couple of changes. I put the comment with the data that define the cases for this change rather than on the generic test method. I also changed the assertion in the test method to assertThat(buildRef(pathA).equals(buildRef(pathB)), is(expectToBeEqual)) so the call on equals is explicit rather than implicit. Please don't hesitate to send me back for another round if you see any other changes worth making. You can also consider doing a follow-on PR to suggest small changes.

[ggalmazor]

Hi!

I also changed the assertion in the test method to assertThat(buildRef(pathA).equals(buildRef(pathB)), is(expectToBeEqual)) so the call on equals is explicit rather than implicit

I don't think this is a good idea... Hamcrest matchers work better when all the assertion is in your matchers. By doing an equals in the left hand-side, you're taking that out from the assertion.

The consequence is that now, when the test fails, it gives much less information than before. Now a boolean is not true/false, instead of a object not being equal to another.

If you want to make it explicit that we're "equalling" two objects, you could use the Matchers.equalTo instead of Matchers.is (is is more ideomatic on Hamcrest, and it does the same thing as equalTo, but it's fine to use it anyway IMO).

You could do this:

@Test
public void treeReferenceEqualsTest() {
    Matcher<TreeReference> matcher = equalTo(buildRef(pathB));
    if (!expectToBeEqual)
        matcher = not(matcher);
    assertThat(buildRef(pathA), matcher);
}

(or, you know, use a ternary expression ;) )

[lognaturel]

The consequence is that now, when the test fails, it gives much less information than before. Now a boolean is not true/false, instead of a object not being equal to another.

Yes, but what we want to test is the result of the equals method. Incidentally we can use the is matcher to implicitly call that but I realized it's not very consistent with how we'd verify the return value of a method with any other return type.

A failed test using is would show something like

Expected: is <instance(foo)/foo/bar>
     but: was </foo/bar>

which doesn't particularly correspond to what we're trying to test. On the other hand,

Expected: is <false>
     but: was <true>

makes it clear that we expected the result of an equality check to be false but it was true for some reason.

Anyway, hopefully these tests are more of a formality than anything else so if you prefer using is even with this explanation, that's ok by me! It is what I went for initially for the idiomatic reasons you described but then I realized what it was testing wasn't quite right.

[ggalmazor]

LGTM!