apache / iceberg

Apache Iceberg
https://iceberg.apache.org/
Apache License 2.0
6.42k stars 2.22k forks source link

Add checkstyle rule to ensure AssertJ assertions always check for underlying exception message #7040

Open nastra opened 1 year ago

nastra commented 1 year ago

Feature Request / Improvement

We should ensure that test code asserting on exceptions should always check for the underlying exception message as shown below:

// should fail because it doesn't check for the underlying message
Assertions.assertThatThrownBy(() -> ...)
        .isInstanceOf(ValidationException.class);

// should not fail as it checks for the exception message
Assertions.assertThatThrownBy(() -> ...)
        .isInstanceOf(ValidationException.class)
        .hasMessageContaining(...);

// should fail because it doesn't check for the underlying message
Assertions.assertThatExceptionOfType(NotFoundException.class)
        .isThrownBy(() -> ...);

// should not fail as it checks for the exception message
Assertions.assertThatExceptionOfType(NotFoundException.class)
        .isThrownBy(() -> ...)
        .withMessageContaining(...);

Query engine

None

tobehardest commented 1 year ago

Can you assign this task to me?

nastra commented 1 year ago

thanks for taking this @tobehardest

tobehardest commented 1 year ago

I'm a newbie and I'm having some trouble figuring this out, maybe I need some help.

coded9 commented 1 year ago

@tobehardest we can work together, if you are finding it difficult.

tobehardest commented 1 year ago

@tobehardest we can work together, if you are finding it difficult.

OK. How to contact you?

coded9 commented 1 year ago

Are you on iceberg slack channel ?

tobehardest commented 1 year ago

Yes, I am on.

tobehardest commented 1 year ago

My name inside is, Yikun Chen.

github-actions[bot] commented 9 months ago

This issue has been automatically marked as stale because it has been open for 180 days with no activity. It will be closed in next 14 days if no further activity occurs. To permanently prevent this issue from being considered stale, add the label 'not-stale', but commenting on the issue is preferred when possible.