PP-12107: Move BinRangeTest from pay-cardid-data

[oswaldquek]

Moving BinRangeTest from pay-cardid-data into this repo as the build-and-push-cardid-candidate CI run will copy the real bin ranges files from s3 into src/main/resources/data-sources. Thus the test will run against real card data in a CI build.

To make the test work locally, overlapping ranges in the current test data (in src/main/resources/data-sources) needed to be amended/deleted.

Also removed the long comment in CardIdResourceITest as it claims the usefulness of it's tests isn't clear. On the contrary the test is useful as it verifies the API actually works.

Removed -DskipTests from the Dockerfile as we want to verify the bin ranges don't overlap (via running BinRangesTest) whilst building the Docker image.

[DomBelcher]

A few questions, rather than anything that definitively needs changing

[DomBelcher]

Is there any reason this is a Function object vs. just being a private static method?

[oswaldquek]

Hmm yeah it could be a private static field, I'll make the change.

[oswaldquek]

Although, given this is just test code, it doesn't really matter that we're instantiating this cardRangeExtractor on each thread, rather than placing it in memory reserved for private static fields.

[DomBelcher]

I think this test doesn't do anything useful? It builds up the RangeSet and the HashMap from the ranges, and checks that every range in the RangeSet is in the HashMap, but that will always be the case, regardless of what the ranges are, because the ranges are added to both at the same time.

[DomBelcher]

If you wanted to test that none of the ranges overlap, you could do something like

@Test
void verifyNoOverlappingRanges() {
    RangeSet<Long> setOfBinRanges = TreeRangeSet.create();
    List<Range<Long>> overlappingRanges = new ArrayList<>();
    
    worldpayData.forEach(cardRange -> {
        boolean contains = setOfBinRanges.intersects(cardRange);
        if (contains) {
            overlappingRanges.add(cardRange);
        }
        
        setOfBinRanges.add(cardRange);
    });
    discoverData.forEach(cardRange -> {
        boolean contains = setOfBinRanges.intersects(cardRange);
        if (contains) {
            overlappingRanges.add(cardRange);
        }
        
        setOfBinRanges.add(cardRange);
    });
    testCardData.forEach(cardRange -> {
        boolean contains = setOfBinRanges.intersects(cardRange);
        if (contains) {
            overlappingRanges.add(cardRange);
        }
        
        setOfBinRanges.add(cardRange);
    });
    assertThat(overlappingRanges.size(), is(0));
}

although this somewhat mirrors the other tests - it does have the advantage of checking data within a range, not just between ranges

[DomBelcher]

Digging into the history it seems this test used to be called checkIfNoExistingRangeInHashMap - I'm still not clear on what the point of that test is though?

[oswaldquek]

I think this test doesn't do anything useful? It builds up the RangeSet and the HashMap from the ranges, and checks that every range in the RangeSet is in the HashMap, but that will always be the case, regardless of what the ranges are, because the ranges are added to both at the same time.

If there are overlapping ranges, RangeSet will adjust the sets of ranges accordingly, and it will mean there will be a difference in the range sets between the RangeSet and HashMap. See how it works at https://guava.dev/releases/23.0/api/docs/com/google/common/collect/RangeSet.html. So I think your suggested code, although arguably clearer in intention, and what is currently here are essentially the same thing.

[oswaldquek]

Digging into the history it seems this test used to be called checkIfNoExistingRangeInHashMap - I'm still not clear on what the point of that test is though?

Nat and I weren't clear on what the was checkIfNoExistingRangeInHashMap trying to achieve, so we renamed it verifyNoOverlappingRanges and gave it some javadoc to clarify. See https://github.com/alphagov/pay-cardid-data/pull/68/files#diff-b5d0a0f2d9eca8d9c1aea690c8dce8ab047b489f2d53c34b28987ec6f5bc31a3R56-R65

[DomBelcher]

👍