test: add lightweight unit coverage for EnrichmentPipeline #138

[davide954]

This PR adds a pure JUnit test (EnrichmentPipelineTest.java) for the EnrichmentPipeline class.

Instead of spinning up a full SparkSession (which is slow and can have path issues on Windows), this test:

1. Manually creates input Row objects.
2. Runs them through the pipeline logic.
3. Verifies the output.

Passes with mvn clean test.

[davide954]

Addressed the feedback from the previous review.

I updated the test to use @ParameterizedTest with @CsvSource as suggested.

Regarding your comment about CURColumn: I noticed that some standard columns (like dates, costs, and IDs) were missing from the class. I added them to CURColumn.java so we can use the constants in the test instead of hardcoded strings.

[jnioche]

Thanks, will review more when I have more time

Regarding your comment about CURColumn: I noticed that some standard columns (like dates, costs, and IDs) were missing from the class. I added them to CURColumn.java so we can use the constants in the test instead of hardcoded strings.

I think we should have in CURColumn only things that are used in the enrichment modules. The tests should also not use fields that are not needed by the modules, likeline_item_unblendedcost - did you want to add those for a specific purpose?

[tejas-2232]

Great work @davide954 🎆
I would like to request some minor changes.

[davide954]

Thanks @tejas-2232.
@jnioche I'll stay in touch for your instructions...

[tejas-2232]

line 55-57.
The hardcoded array indices depend on loop iteration order for enrichment module columns. This looks fragile. what do you think about adding something like this?

int regionIndex = schema.fieldIndex(CURColumn.PRODUCT_REGION_CODE.getLabel());
int lineItemIndex = schema.fieldIndex(CURColumn.LINE_ITEM_TYPE.getLabel());
values[regionIndex] = "us-east-1";
values[lineItemIndex] = lineItemType;

[davide954]

Great catch! That makes perfect sense I'll work on it.

[tejas-2232]

Missing Null Check on fieldIndex()

fieldIndex() can throw IllegalArgumentException if field doesn't exist.

I would say -> Add try-catch or ensure field exists first

try {
    int fieldIndex = processedRow.fieldIndex(c.getLabel());
    // assertions...
} catch (IllegalArgumentException e) {
    fail("Column " + c.getLabel() + " not found in schema: " + e.getMessage());
}

[davide954]

I thought about it, but I wonder if the explicit try-catch is necessary here. Since this is a unit test, if fieldIndex() throws an IllegalArgumentException, the test will automatically fail with the exception message (which already identifies the missing field).

I'd prefer to keep the loop clean and let JUnit handle the exception as a test failure, unless you have a specific case in mind where masking the exception is better? In this case enlighten me

[tejas-2232]

I thought about it, but I wonder if the explicit try-catch is necessary here. Since this is a unit test, if fieldIndex() throws an IllegalArgumentException, the test will automatically fail with the exception message (which already identifies the missing field).

I'd prefer to keep the loop clean and let JUnit handle the exception as a test failure, unless you have a specific case in mind where masking the exception is better? In this case enlighten me

haha, right. I went too far in thinking. That works anytime.

[tejas-2232]

No Assertion on Module Count:
If config.getModules() returns an empty list then the test passes without actually validating anything.

I would say - you can add below assertion right after pipeline is created.

Let me know what you think? 😃

List<EnrichmentModule> modules = config.getModules();

assertTrue(!modules.isEmpty(), "Config should have at least one enrichment module for this test");
// OR below one 
assertTrue(modules.size() > 0, "Config should have at least one enrichment module for this test");

[davide954]

I'll push tomorrow! Now I'm signing off as it's almost midnight here in Italy. Thanks for the sharp review @tejas-2232, catch you later!