Skip to content
This repository was archived by the owner on Jan 24, 2024. It is now read-only.

[CI] Speed up CI test and fix flaky test#1925

Merged
BewareMyPower merged 20 commits intostreamnative:masterfrom
yzang2019:yzang/fix-flaky-test
Jul 4, 2023
Merged

[CI] Speed up CI test and fix flaky test#1925
BewareMyPower merged 20 commits intostreamnative:masterfrom
yzang2019:yzang/fix-flaky-test

Conversation

@yzang2019
Copy link
Contributor

@yzang2019 yzang2019 commented Jun 26, 2023

Motivation

KoP test is too slow, and there are couple of test very flaky and always timeout.

Modifications

Parallelize KoP uni tests with multiple jobs after regroup the tests into corresponding packages
Add a testing matrix for github workflow to spin up one job for each group of tests
Increase timeout to fix some flaky tests

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Documentation

Check the box below.

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions
Copy link

@yzang2019:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@github-actions github-actions bot added doc-info-missing This pr needs to mark a document option in description and removed doc-info-missing This pr needs to mark a document option in description labels Jun 26, 2023
@github-actions
Copy link

@yzang2019:Thanks for providing doc info!

@github-actions github-actions bot added the no-need-doc This pr does not need any document label Jun 26, 2023
}

@Test(timeOut = 1000 * 10, dataProvider = "produceConfigProvider")
@Test(timeOut = 1000 * 30, dataProvider = "produceConfigProvider")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've run these locally and it takes 22 seconds

}

@Test(timeOut = 20000, expectedExceptions = KeeperException.NoNodeException.class)
@Test(timeOut = 60000, expectedExceptions = KeeperException.NoNodeException.class)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested this locally which takes usually 20-30 seconds to finish

Demogorgon314
Demogorgon314 previously approved these changes Jun 27, 2023
@BewareMyPower
Copy link
Collaborator

I don't think it speeds up the CI test actually.

image

You can see the module-test, i.e. the tests under the tests/ directory, took the most of the time of CI (44 minutes), while the other tests only took 14 minutes in summary.

See another CI example in #1914:

image

How could you say this PR speeds up the CI test?

@gaoran10
Copy link
Contributor

I don't think it speeds up the CI test actually.

Maybe we need to add some test groups for tests and use different ci jobs to test various test groups.

@BewareMyPower
Copy link
Collaborator

Maybe we need to add some test groups for tests

+1

@yzang2019 yzang2019 changed the title Speed up CI test and fix flaky test [CI] Speed up CI test and fix flaky test Jun 28, 2023
@yzang2019
Copy link
Contributor Author

I don't think it speeds up the CI test actually.

image You can see the `module-test`, i.e. the tests under the `tests/` directory, took the most of the time of CI (44 minutes), while the other tests only took 14 minutes in summary.

See another CI example in #1914:

image How could you say this PR speeds up the CI test?

Yeah, you are right, I only realized this after I split it into multiple jobs, I think next step is to split them into groups

@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Merging #1925 (b02bd73) into master (f3b95a2) will decrease coverage by 0.70%.
The diff coverage is 3.52%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1925      +/-   ##
============================================
- Coverage     18.64%   17.94%   -0.70%     
- Complexity      743      745       +2     
============================================
  Files           186      194       +8     
  Lines         13377    13957     +580     
  Branches       1231     1291      +60     
============================================
+ Hits           2494     2505      +11     
- Misses        10702    11271     +569     
  Partials        181      181              
Impacted Files Coverage Δ
...streamnative/pulsar/handlers/kop/DelayedFetch.java 0.00% <0.00%> (ø)
...ative/pulsar/handlers/kop/KafkaCommandDecoder.java 0.32% <0.00%> (-0.01%) ⬇️
...tive/pulsar/handlers/kop/KafkaProtocolHandler.java 0.00% <0.00%> (ø)
...ative/pulsar/handlers/kop/KafkaRequestHandler.java 1.07% <0.00%> (-0.01%) ⬇️
...pulsar/handlers/kop/KafkaTopicConsumerManager.java 0.00% <0.00%> (ø)
...e/pulsar/handlers/kop/KafkaTopicLookupService.java 2.00% <0.00%> (ø)
...ndlers/kop/coordinator/group/GroupCoordinator.java 0.00% <0.00%> (ø)
...rs/kop/coordinator/transaction/PendingRequest.java 0.00% <0.00%> (ø)
...kop/coordinator/transaction/TransactionConfig.java 0.00% <0.00%> (ø)
...oordinator/transaction/TransactionCoordinator.java 0.00% <0.00%> (ø)
... and 23 more

@yzang2019
Copy link
Contributor Author

@BewareMyPower @Demogorgon314 I've addressed comments, total test time dropped from 50 minutes to around 10 minutes now, could you take another look?

Copy link
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

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

Great work! LGTM, left a trivial comment.


@Cleanup
final KafkaProducer<String, String> kafkaProducer = newKafkaProducer();
@Cleanup final KafkaProducer<String, String> kafkaProducer = newKafkaProducer();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we retain the code format?

Copy link
Collaborator

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

Great work!

@BewareMyPower BewareMyPower merged commit 24180bb into streamnative:master Jul 4, 2023
BewareMyPower added a commit to BewareMyPower/kop that referenced this pull request Jul 31, 2023
### Motivation

After streamnative#1925, the surefire
artifacts won't be uploaded if tests failed.

### Modifications

Upload the surefire artifacts when tests failed.
BewareMyPower added a commit that referenced this pull request Aug 1, 2023
### Motivation

After #1925, the surefire
artifacts won't be uploaded if tests failed.

### Modifications

Upload the surefire artifacts when tests failed.
Demogorgon314 pushed a commit to Demogorgon314/kop that referenced this pull request Aug 14, 2023
### Motivation

KoP test is too slow, and there are couple of test very flaky and always
timeout.

### Modifications

Parallelize KoP uni tests with multiple jobs after regroup the tests
into corresponding packages
Add a testing matrix for github workflow to spin up one job for each
group of tests
Increase timeout to fix some flaky tests

(cherry picked from commit 24180bb)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-need-doc This pr does not need any document release/2.11 release/3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants