MINOR: Update JUnit to 4.13 and annotate log cleaner integration test by ijuma · Pull Request #6248 · apache/kafka

ijuma · 2019-02-10T08:06:02Z

JUnit 4.13 fixes the issue where Category and Parameterized annotations
could not be used together. It also deprecates ExpectedException and
assertThat. Given this, we:

Replace ExpectedException with the newly introduced assertThrows.
Replace Assert.assertThat with MatcherAssert.assertThat.
Annotate AbstractLogCleanerIntegrationTest with IntegrationTest category.

Committer Checklist (excluded from commit message)

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

ijuma · 2019-02-10T08:11:30Z

#3333 and #3977 were two previous attempts at annotating the log cleaner integration test.

ijuma · 2019-02-10T18:58:01Z

Relevant sections of gradle's profile report that take more than 5 seconds:

:core | 1m56.36s | (total)
:core:compileScala | 47.057s |
:core:unitTest | 38.768s |
:core:compileTestScala | 29.112s

:clients | 1m18.65s | (total)
:clients:unitTest | 1m10.78s |

connect:runtime | 48.457s | (total)
:connect:runtime:unitTest | 46.623s |

:streams | 47.635s | (total)
:streams:unitTest | 40.710s |

tools | 30.031s | (total)
:tools:unitTest | 28.552s

:streams:streams-scala | 16.430s | (total)
:streams:streams-scala:unitTest | 12.939s |

ewencp

A couple of comments, but mostly looks good as it is mostly a mechanical change.

You got my hopes up with this, I've been refreshing the junit 4.13 release issue daily waiting for beta to be approved and released as its final version :)

ewencp · 2019-02-10T20:56:37Z

-            exceptionRule.expectMessage("ZStandard compression is not supported for magic " + magic);
-        }
+    private void assumeV2OrNotZstd(byte magic) {
+        assumeTrue(compressionType != CompressionType.ZSTD || magic == MAGIC_VALUE_V2);


why the shift to checking exact v2 rather than that it is >= if zstd? given our default would be to preserve the feature moving forward, it seems better to write these tests to work for future updates by default (as appears to be the case with the code previously) and only have to edit them if we made a change that wasn't compatible with v2 zstd support.

Just an oversight, I think I did it right in the other class. Will fix.

ewencp · 2019-02-10T21:18:27Z

    public void testFilterToBatchDiscard() {
-        if (compression != CompressionType.NONE || magic >= RecordBatch.MAGIC_VALUE_V2) {
-            expectExceptionWithZStd(compression, magic);
+        if ((compression != CompressionType.NONE && compression != CompressionType.ZSTD) ||


If we're starting to use Assume, this seems like it should be one since the condition just skips the whole test?

I was thinking the same, but didn't do it in the end. Will change.

ijuma · 2019-02-10T21:52:14Z

@ewencp I was refreshing it regularly too and eventually decided that we don't need to wait for it since the rate of change in that branch is nearly 0 and the beta is in the central Maven repo. We'll just bump to final when that happens.

ijuma · 2019-02-10T23:15:02Z

JDK 11 build passed, JDK 8 build had one flake:

java.lang.AssertionError:
Expected: <[KeyValue(0, 0), KeyValue(0, 1), KeyValue(0, 3), KeyValue(0, 6), KeyValue(0, 10), KeyValue(0, 15), KeyValue(0, 21), KeyValue(0, 28), KeyValue(0, 36), KeyValue(0, 45), KeyValue(0, 55), KeyValue(0, 66), KeyValue(0, 78), KeyValue(0, 91), KeyValue(0, 105), KeyValue(0, 120), KeyValue(0, 136), KeyValue(0, 153), KeyValue(0, 171), KeyValue(0, 190)]>
but: was <[KeyValue(0, 0), KeyValue(0, 1), KeyValue(0, 3), KeyValue(0, 6), KeyValue(0, 10), KeyValue(0, 15), KeyValue(0, 21), KeyValue(0, 28), KeyValue(0, 36), KeyValue(0, 45), KeyValue(0, 10), KeyValue(0, 21), KeyValue(0, 33), KeyValue(0, 46), KeyValue(0, 60), KeyValue(0, 75), KeyValue(0, 91), KeyValue(0, 108), KeyValue(0, 126), KeyValue(0, 145)]>
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
at org.apache.kafka.streams.integration.EosIntegrationTest.checkResultPerKey(EosIntegrationTest.java:212)
at org.apache.kafka.streams.integration.EosIntegrationTest.shouldNotViolateEosIfOneTaskFailsWithState(EosIntegrationTest.java:440)

cc @guozhangwang @mjsax

ijuma · 2019-02-11T00:06:20Z

@ewencp I've addressed the comments and fixed a few more things I noticed.

ewencp

One last nit on the version checking in one of the tests, but otherwise LGTM (modulo getting a clean, non-timed-out build)

ewencp · 2019-02-11T17:54:58Z

    }

+    private void assumeV2OrNotZstd() {
+        assumeTrue(compression != CompressionType.ZSTD || magic == MAGIC_VALUE_V2);


magic comparison to v2 has same equality change here as in the previous test. any reason not to >=?

Sorry for missing this one, fixed.

mumrah

LGTM, one small comment inline

mumrah · 2019-02-11T19:31:54Z

            Thread.currentThread().interrupt();
-            expectedException.expect(InterruptException.class);
-            consumer.poll(Duration.ZERO);
+            assertThrows(InterruptException.class, () -> consumer.poll(Duration.ZERO));


Oh wow, this is so much nicer than the rule thing or explicit catches. +1

mumrah · 2019-02-11T19:33:54Z

-        MemoryRecords records = builder.build();
-        assertEquals(0, records.sizeInBytes());
-        assertEquals(bufferOffset, buffer.position());
+        Supplier<MemoryRecordsBuilder> builderSupplier = () -> new MemoryRecordsBuilder(buffer, magic,


Since the build method of the builder is what you're testing here, I don't think the Supplier is necessary.

The checks happen in the constructor, right?

if (magic < RecordBatch.MAGIC_VALUE_V2) { if (isTransactional) throw new IllegalArgumentException("Transactional records are not supported for magic " + magic); if (isControlBatch) throw new IllegalArgumentException("Control records are not supported for magic " + magic); if (compressionType == CompressionType.ZSTD) throw new IllegalArgumentException("ZStandard compression is not supported for magic " + magic); }

Gotcha. I (incorrectly) assumed the "build" method.

ijuma · 2019-02-11T22:34:54Z

retest this please

ijuma · 2019-02-12T02:32:36Z

retest this please

ijuma · 2019-02-12T06:04:52Z

Tests passed, merging to trunk.

…apache#6248) JUnit 4.13 fixes the issue where `Category` and `Parameterized` annotations could not be used together. It also deprecates `ExpectedException` and `assertThat`. Given this, we: - Replace `ExpectedException` with the newly introduced `assertThrows`. - Replace `Assert.assertThat` with `MatcherAssert.assertThat`. - Annotate `AbstractLogCleanerIntegrationTest` with `IntegrationTest` category. Reviewers: Ewen Cheslack-Postava <ewen@confluent.io>, David Arthur <mumrah@gmail.com>

ijuma requested review from ewencp, mumrah and omkreddy February 10, 2019 08:06

ijuma mentioned this pull request Feb 10, 2019

MINOR: Improve PlainSaslServer error message for empty tokens #6249

Merged

3 tasks

ijuma added 2 commits February 10, 2019 09:56

Upgrade to JUnit 4.13-beta2

3fad47f

Annotate as IntegrationTest

958b169

ijuma force-pushed the junit-4.13 branch from 3a68e47 to 958b169 Compare February 10, 2019 17:56

ewencp reviewed Feb 10, 2019

View reviewed changes

ijuma added 3 commits February 10, 2019 15:34

Address review comments

74dbb61

Use method references instead of lambdas whenever possible

9c50f3e

Fix assertEquals order

295c499

ijuma force-pushed the junit-4.13 branch from 0e28e40 to 295c499 Compare February 10, 2019 23:51

ewencp approved these changes Feb 11, 2019

View reviewed changes

Address review feedback

31469cf

mumrah approved these changes Feb 11, 2019

View reviewed changes

ijuma merged commit c7f99bc into apache:trunk Feb 12, 2019

ijuma deleted the junit-4.13 branch February 12, 2019 06:06

omkreddy mentioned this pull request Feb 12, 2019

MINOR: Remove deprecated assertThat method usage from KafkaLog4jAppenderTest #6257

Merged

3 tasks

ewencp mentioned this pull request Mar 17, 2019

KAFKA-8058: Fix ConnectClusterStateImpl.connectors() method #6384

Merged

3 tasks

Conversation

ijuma commented Feb 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

ijuma commented Feb 10, 2019

Uh oh!

ijuma commented Feb 10, 2019

Uh oh!

ewencp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Feb 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijuma commented Feb 10, 2019

Uh oh!

ijuma commented Feb 11, 2019

Uh oh!

ewencp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mumrah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Feb 11, 2019

Uh oh!

ijuma commented Feb 12, 2019

Uh oh!

ijuma commented Feb 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ijuma commented Feb 10, 2019 •

edited

Loading

ijuma commented Feb 10, 2019 •

edited

Loading