Filter entities from comparator combiner when not listed in input_features #3251
Conversation
|
Wondering why this behavior is not enforced by this validation check: ludwig/ludwig/config_validation/checks.py Line 194 in d5f61eb |
|
@ksbrar that would validate the config, but this PR makes the behavior more permissive by letting these configs go through. |
|
Ah gotcha, was thinking backwards - purpose IS to make the parameter more permissive 👍 |
| }, | ||
| } | ||
|
|
||
| with pytest.raises(ConfigValidationError) if not expected else contextlib.nullcontext(): |
There was a problem hiding this comment.
Maximally parameterizing tests to minimize code duplication is tempting, but it can make tests more difficult to read and understand when it involves adding logic to the test itself.
See "Avoid logic in tests" from Microsoft's testing best practices guide.
The behavior we are testing for is more readable/understandable as three separate tests, imo:
def test_comparator_combiner_raises_missing_features():
def test_comparator_combiner_removes_extra_features():
def test_comparator_combiner_raises_duplicated_features():There was a problem hiding this comment.
Can't say I agree. I was thinking of adding a handful of additioanl tests to this. Parametrizing makes this super easy. Separate tests means in practice, no one's ever going to bother.
There was a problem hiding this comment.
The good thing is that the relatively simple logic in this test means it's already pretty readable.
Now that there are 7 tests, I would agree that splitting into separate tests would be too much code duplication and not worthwhile.
As a nit: would you be able to add a quick comment do the different lines explaining what each case is testing for?
No description provided.