Change tests not to use "fail() + catch" pattern

[findepi]

Some tests use the following pattern

try {
    ... do something ...
    fail();
}
catch (IllegalStateException e) {
    assertEquals(e.getMessage(), "the expected message");
}

this is not very readable and can be expressed more tersely with assertThatThrownBy also, in case the message not as expected, the actual failure gets lots, so it's harder to diagnose the problem

find all occurrences of such pattern and migrate to assertThatThrownBy see https://github.com/trinodb/trino/pull/6554 for example of such migration

[choiwaiyiu]

Hi @findepi May I work on this?

[findepi]

@choiwaiyiu sure, thank you!

[findepi]

please make sure to have a look at https://github.com/trinodb/trino/pull/6554 to see how some of the code places have been updated recently.

[MariyaR]

Hello @findepi, does @choiwaiyiu still work on that issue or I can try?

[findepi]

@MariyaR hopefully @choiwaiyiu will clarify that. If not, please feel free to go ahead.

[choiwaiyiu]

@MariyaR @findepi Sorry for my slow progress. I have already started working on this issue and will create a pull request soon.

[findepi]

@choiwaiyiu no worries and thanks for info!

Also, thank you for your #6767 PR! @MariyaR do you want to help review this?

[ebyhr]

@findepi Should we prohibit fail() by modernizer?

[findepi]

I think there are legitimate cases where verifying some condition.

We shouldn't need to

assertTrue(some condition);

as

if (! some condition) {
  fail("good message");
}

reads better.

Of course, we could use throw new AssertionError in these cases, but we should make sure we have an agreement on this first.

[MariyaR]

@findepi yes, I would like to help with review

[findepi]

https://github.com/trinodb/trino/pull/6767 is merged. @choiwaiyiu can you please do another check whether there is anything remaining and close the issue? thanks!

[choiwaiyiu]

@findepi Here are the remaining "fail() + catch" pattern.

• trino-blackhole

  • TestBlackHoleSmoke
  • https://github.com/trinodb/trino/blob/master/plugin/trino-blackhole/src/test/java/io/trino/plugin/blackhole/TestBlackHoleSmoke.java#L112-L132

• trino-hive

  • AbstractTestHive

  • https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java#L1186-L1205

  • https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java#L1456-L1462

  • https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java#L1484-L1491

  • https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java#L1505-L1513

  • https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java#L2085-L2090

  • https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java#L2338-L2347

  • https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java#L2338-L2347

  • https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java#L2682-L2692

  • https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java#L3323-L3329

  • https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java#L3349-L3356

  • https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java#L3955-L3965

  • https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java#L4570-L4579

  • TestFSDataInputStreamTail

  • https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/util/TestFSDataInputStreamTail.java#L163-L170

  • https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/util/TestFSDataInputStreamTail.java#L184-L187 (no catch block)

  • TestHiveTypeTranslator

  • https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/util/TestHiveTypeTranslator.java#L87-L100

• trino-main

  • FunctionAssertions (used inside a deprecated method)

  • https://github.com/trinodb/trino/blob/master/core/trino-main/src/test/java/io/trino/operator/scalar/FunctionAssertions.java#L310-L321

  • TestClientBuffer

  • https://github.com/trinodb/trino/blob/master/core/trino-main/src/test/java/io/trino/execution/buffer/TestClientBuffer.java#L362-L380

  • https://github.com/trinodb/trino/blob/master/core/trino-main/src/test/java/io/trino/execution/buffer/TestClientBuffer.java#L382-L402

• trino-phoenix5

  • TestPhoenixConnectorTest
  • https://github.com/trinodb/trino/blob/master/plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java#L201-L206

• trino-product-tests

  • TestSqlCancel
  • https://github.com/trinodb/trino/blob/master/testing/trino-product-tests/src/main/java/io/trino/tests/TestSqlCancel.java#L122-L133

• trino-prometheus

  • TestPrometheusIntegrationSchema
  • https://github.com/trinodb/trino/blob/master/plugin/trino-prometheus/src/test/java/io/trino/plugin/prometheus/TestPrometheusIntegrationSchema.java#L117-L122
  • https://github.com/trinodb/trino/blob/master/plugin/trino-prometheus/src/test/java/io/trino/plugin/prometheus/TestPrometheusIntegrationSchema.java#L123-L128

- trino-spi

- TestUnscaledDecimal128Arithmetic - https://github.com/trinodb/trino/blob/master/core/trino-spi/src/test/java/io/trino/spi/type/TestUnscaledDecimal128Arithmetic.java#L631-L644

[rnzucker]

Since choiwaiyiu moved this back to Open, I assume that there is still work to do here? I wasn't sure if choiwaiyiu's Mar. 22nd update means that there are still cases to fix.

[Faizankhan1104]

Hello, @findepi I would like to start my contribution to this issue. Can I pick this up??