MINOR: Replace null with an actual value for timestamp field in InsertField SMT unit tests

[wicknicks]

Signed-off-by: Arjun Satish arjun@confluent.io

PR adds a unit test to check the behavior of the InsertField SMT when trying to insert a field that takes its value from the Kafka record timestamp. All other tests currently pass a null value for the record timestamp.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[wicknicks]

@rhauch @kkonstantine @C0urante small addition here. reviews appreciated. thanks in advance!

[C0urante]

The code looks good; I'm just a little curious about why we'd want to do a separate test here instead of altering the existing tests to all (or nearly all) use actual timestamp values instead of null?

[wicknicks]

The other tests seemed to be testing for the empty timestamp case. Hence the new test, also helps keep the new test precise to a usecase. I'd prefer to just add another case for the schema case, if that is the main concern that made you thin that way, @C0urante. WDYT?

[C0urante]

I think changing the empty timestamp cases to use a real timestamp instead might be fine here. The ? or ! suffix logic is the same for the timestamp field as it is with the partition, offset, topic, and static fields.
If we want to explicitly test for the optionality of a field based on that suffix that might call for a separate test on its own, but it doesn't seem like there's anything special about the logic or the code for the timestamp field that would necessitate an entirely separate test for it.

[wicknicks]

@C0urante updated the test. it should cover both schema and schemaeless cases in existing tests. let me know if that works.

[C0urante]

LGTM, thanks Arjun!

[kkonstantine]

@wicknicks I understand that the current tests were taking a shortcut by not filling out a the timestamp field with an actual value. But other than observing this omission, is there another motivation behind this change that we'd need to take into account and include in the description of this PR?

[wicknicks]

@kkonstantine a Connector was not setting the field correctly in a local setup, and I was digging into why this is the case. to check if the SMT is correct, I looked at the tests, and they were all checking nulls, as you pointed out. Hence, the update, so we have clarity on what the expected input is, and what the output should be.

[kkonstantine]

Sounds good. Thanks for improving these tests.

LGTM

[kkonstantine]

jdk14 green, jdk 11 failed on 1 flaky test, jdk 8 failed to run, but that's ok.

[wicknicks]

Thanks, @kkonstantine and @C0urante!