Skip to content

MINOR: Tag RaftEventSimulationTest as integration and tweak it#9925

Merged
ijuma merged 2 commits intoapache:trunkfrom
ijuma:tweak-raft-event-simulation-test
Jan 23, 2021
Merged

MINOR: Tag RaftEventSimulationTest as integration and tweak it#9925
ijuma merged 2 commits intoapache:trunkfrom
ijuma:tweak-raft-event-simulation-test

Conversation

@ijuma
Copy link
Copy Markdown
Member

@ijuma ijuma commented Jan 18, 2021

The test takes over 1 minute to run, so it should not be considered a
unit test.

Also:

  • Replace test prefix with check prefix for helper methods. A common
    mistake is to forget to add the @test annotation, so it's good to use a
    different naming convention for methods that should have the annotation
    versus methods that should not.
  • Replace Action functional interface with built-in Runnable.
  • Remove unnecessary assumeTrue.
  • Remove @FunctionalInterface from Invariant since it's not used
    in that way.

Committer Checklist (excluded from commit message)

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

The test takes over 1 minute to run, so it should not be considered a
unit test.

Also:
* Replace `test` prefix with `check` prefix for helper methods. A common
mistake is to forget to add the @test annotation, so it's good to use a
different naming convention to methods that should have the annotation
versus methods that should not.
* Replace `Action` functional interface with built-in `Runnable`.
@ijuma ijuma requested review from abbccdda and hachikuji January 18, 2021 16:32
Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ijuma , I love the refactor to replace Action functional interface with built-in Runnable. LGTM!

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other functional interfaces (Validation and Invariant). The setter (addInvariant and addValidation) should be clear enough so we don't need to keep those function interfaces. WDTY?

@@ -59,6 +60,7 @@
import static org.junit.jupiter.api.Assertions.fail;
import static org.junit.jupiter.api.Assumptions.assumeTrue;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is assumeTrue used in this test? I test ```RaftEventSimulationTest`` but there is no ignored test cases.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, removed since it's not currently needed.

@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Jan 22, 2021

@chia7712 What are you suggesting instead of Validation and Invariant? Runnable?

@chia7712
Copy link
Copy Markdown
Member

What are you suggesting instead of Validation and Invariant? Runnable?

Runnable

@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Jan 22, 2021

I removed the @FunctionalInterface annotation from Invariant. I don't think this interfaces are used as functional interfaces and they make the system more strongly typed. Action, on the other hand, was playing a very similar role to Runnable and hence why I thought it was not worth keeping.

@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Jan 23, 2021

@chia7712 Does this look ok to you? I am interested in making unitTest fast, so would like to get this merged.

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ijuma ijuma merged commit 6f8ca66 into apache:trunk Jan 23, 2021
@ijuma ijuma deleted the tweak-raft-event-simulation-test branch January 23, 2021 23:57
ijuma added a commit to ijuma/kafka that referenced this pull request Jan 26, 2021
…e-allocations-lz4

* apache-github/trunk: (562 commits)
  MINOR: remove unused code from MessageTest (apache#9961)
  MINOR: Fix visibility of Log.{unflushedMessages, addSegment} methods (apache#9966)
  KAFKA-12229: Restore original class loader in integration tests using EmbeddedConnectCluster during shutdown  (apache#9942)
  KAFKA-12190: Fix setting of file permissions on non-POSIX filesystems (apache#9947)
  MINOR: Remove `toStruct` and `fromStruct` methods from generated protocol classes (apache#9960)
  MINOR: Fix typo in Utils#toPositive (apache#9943)
  MINOR: MessageUtil: remove some deadcode (apache#9931)
  MINOR: Update zstd-jni to 1.4.8-2 (apache#9957)
  MINOR: Revert assertion in MockProducerTest (apache#9956)
  MINOR: Optimize assertions in unit tests (apache#9955)
  MINOR: Tag `RaftEventSimulationTest` as `integration` and tweak it (apache#9925)
  MINOR: Update to Gradle 6.8.1 (apache#9953)
  MINOR: A few small group coordinator cleanups (apache#9952)
  MINOR: Upgrade ducktape to version 0.8.1  (apache#9933)
  MINOR: fix record time in test shouldWipeOutStandbyStateDirectoryIfCheckpointIsMissing (apache#9948)
  MINOR: Restore interrupt status when closing (apache#9863)
  KAFKA-10357: Extract setup of repartition topics from Streams partition assignor (apache#9848)
  KAFKA-12212; Bump Metadata API version to remove `ClusterAuthorizedOperations` fields (KIP-700) (apache#9945)
  MINOR: log 2min processing summary of StreamThread loop (apache#9941)
  MINOR: Drop enable.metadata.quorum config (apache#9934)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants