iceberg icon indicating copy to clipboard operation
iceberg copied to clipboard
verified publisher icon apache

Flink: FlinkSink & IcebergSink desynchronized tests alignment

Open arkadius opened this issue 1 year ago • 4 comments

Summary

Currently, the unit tests for IcebergSink (based on the Flink SinkV2 interface) have a code duplication from FlinkSink tests. In the IcebergSink tests there were done some code enhancements that haven't been migrated to FlinkSink tests.

The scope of this change is to migrate these enhancements. List of enhancements:

  • Flink.checkAndGetEqualityFieldIds has been replaced by SinkUtil.checkAndGetEqualityFieldIds
  • as("description") has been added to some assertions

Other improvements that were done:

  • Typo fix in testOperatorsUidNameWitUidSuffix
  • TestFlinkIcebergSinkV2Branch and TestFlinkIcebergSinkV2Base extended TestFlinkIcebergSinkV2Base which has test parameters defined but they weren't used. Instead of that, were used the default, initial values of fields. After this change, test parameters are only defined in leaf classes
  • TestFlinkIcebergSinkV2Base.testChangeLogs has a keySelector parameter that wasn't used anywhere - this change removes it. To discuss: Are tests based on this selector correctly checking what was intended?

arkadius avatar Oct 02 '24 16:10 arkadius

@rodmeneses @pvary please take a look

arkadius avatar Oct 02 '24 16:10 arkadius

@pvary @stevenzwu could you guys please start the CI pipelines on this PR? Thanks

rodmeneses avatar Oct 09 '24 21:10 rodmeneses

This one looks good to me. @pvary could you please add me as reviewer so I can register my approval ? Thanks

rodmeneses avatar Oct 10 '24 16:10 rodmeneses

This one looks good to me. @pvary could you please add me as reviewer so I can register my approval ? Thanks

@pvary can you add @rodmeneses as a reviewer or look at this PR on your own? This is very non-controversial change. Only cleanup of unused code and synchronization of improvements in the code that are already done in other places.

arkadius avatar Oct 21 '24 08:10 arkadius

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

github-actions[bot] avatar Dec 05 '24 00:12 github-actions[bot]