You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "nodece (via GitHub)" <gi...@apache.org> on 2023/06/19 17:32:57 UTC

[GitHub] [pulsar] nodece opened a new pull request, #20611: [cleanup][test] Remove and prevent junit

nodece opened a new pull request, #20611:
URL: https://github.com/apache/pulsar/pull/20611

   ### Motivation
   
   We have been away from `JUnit` for a long time, and use `TestNG` instead, but the `testcontainers` library includes `JUnit` library, so we can still use `JUnit` in our project. 
   
   Let's use `TestNG` uniformly.
   
   ### Modifications
   
   - Prevent `JUnit` by the `checkstyle` file
   - Use `org.testng` instead `org.junit`
   
   ### Documentation
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   - [ ] `doc` <!-- Your PR contains doc changes. -->
   - [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
   - [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
   - [ ] `doc-complete` <!-- Docs have been already added -->
   
   ### Matching PR in forked repository
   
   PR in forked repository: <!-- ENTER URL HERE -->
   
   <!--
   After opening this PR, the build in apache/pulsar will fail and instructions will
   be provided for opening a PR in the PR author's forked repository.
   
   apache/pulsar pull requests should be first tested in your own fork since the 
   apache/pulsar CI based on GitHub Actions has constrained resources and quota.
   GitHub Actions provides separate quota for pull requests that are executed in 
   a forked repository.
   
   The tests will be run in the forked repository until all PR review comments have
   been handled, the tests pass and the PR is approved by a reviewer.
   -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nicoloboschi commented on a diff in pull request #20611: [cleanup][test] Remove and prevent junit

Posted by "nicoloboschi (via GitHub)" <gi...@apache.org>.
nicoloboschi commented on code in PR #20611:
URL: https://github.com/apache/pulsar/pull/20611#discussion_r1235696251


##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/extensions/data/BrokerLookupDataTest.java:
##########
@@ -44,21 +45,21 @@ public void testConstructors() {
                 webServiceUrl, webServiceUrlTls, pulsarServiceUrl,
                 pulsarServiceUrlTls, advertisedListeners, protocols, true, true,
                 ExtensibleLoadManagerImpl.class.getName(), System.currentTimeMillis(),"3.0");
-        Assert.assertEquals(webServiceUrl, lookupData.webServiceUrl());
-        Assert.assertEquals(webServiceUrlTls, lookupData.webServiceUrlTls());
-        Assert.assertEquals(pulsarServiceUrl, lookupData.pulsarServiceUrl());
-        Assert.assertEquals(pulsarServiceUrlTls, lookupData.pulsarServiceUrlTls());
-        Assert.assertEquals(Optional.of("9092"), lookupData.getProtocol("kafka"));
-        Assert.assertEquals(Optional.empty(), lookupData.getProtocol("echo"));
-        Assert.assertTrue(lookupData.persistentTopicsEnabled());
-        Assert.assertTrue(lookupData.nonPersistentTopicsEnabled());
-        Assert.assertEquals("3.0", lookupData.brokerVersion());
+        assertEquals(webServiceUrl, lookupData.webServiceUrl());
+        assertEquals(webServiceUrlTls, lookupData.webServiceUrlTls());

Review Comment:
   IIRC the order of actual and expected are inverted between junit and testng. Could you check if the order is correct now?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #20611: [cleanup][test] Remove and prevent junit

Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #20611:
URL: https://github.com/apache/pulsar/pull/20611#issuecomment-1598184497

   > Please note that the argument order is different in TestNG Assert.assertNotEquals/assertEquals. In TestNG Assert, it's `actual`, `expected`, but in Junit Assert it's `expected`, `actual`. You need to swap the parameter when switching from Junit to TestNG.
   > 
   > (similar comment in review https://github.com/apache/pulsar/pull/14065/files#r795223019)
   
   Thank you for your review, I see this, so I use `AssertJUnit` in some files, and update some calls.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #20611: [cleanup][test] Remove and prevent junit

Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #20611:
URL: https://github.com/apache/pulsar/pull/20611#issuecomment-1601049063

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #20611: [cleanup][test] Remove and prevent junit

Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #20611:
URL: https://github.com/apache/pulsar/pull/20611#issuecomment-1625487824

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #20611: [cleanup][test] Remove and prevent junit

Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #20611:
URL: https://github.com/apache/pulsar/pull/20611#issuecomment-1598027721

   > LGTM.
   > 
   > It's possible to exclude junit dependency from the testcontainers one? 
   
   We can exclude junit from the testcontainers, but we need to add a patch, see this comment from https://github.com/testcontainers/testcontainers-java/issues/970:
   
   > https://github.com/testcontainers/testcontainers-java/issues/970#issuecomment-625044008 and https://github.com/testcontainers/testcontainers-java/issues/970#issuecomment-629479506. Exclude junit4 and add workaround module with the empty classes. Not a perfect solution, but I've been using this setup locally for some time without issues so far.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #20611: [cleanup][test] Remove and prevent junit

Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #20611:
URL: https://github.com/apache/pulsar/pull/20611#issuecomment-1605830503

   /pulsarbot rerun-failure-checks
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #20611: [cleanup][test] Remove and prevent junit

Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #20611:
URL: https://github.com/apache/pulsar/pull/20611#issuecomment-1599985655

   I think we should do our best to ensure the order of the parameters, but In fact, many developers do not pay attention to the order of this parameter when using TestNG/JUnit.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] codecov-commenter commented on pull request #20611: [cleanup][test] Remove and prevent junit

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #20611:
URL: https://github.com/apache/pulsar/pull/20611#issuecomment-1625541037

   ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/20611?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#20611](https://app.codecov.io/gh/apache/pulsar/pull/20611?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (62a045d) into [master](https://app.codecov.io/gh/apache/pulsar/commit/881a1f41202cffaf3b4fd6ab1235fac844d2dddb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (881a1f4) will **decrease** coverage by `0.01%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://app.codecov.io/gh/apache/pulsar/pull/20611/graphs/tree.svg?width=650&height=150&src=pr&token=acYqCpsK9J&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)](https://app.codecov.io/gh/apache/pulsar/pull/20611?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #20611      +/-   ##
   ============================================
   - Coverage     73.14%   73.14%   -0.01%     
   - Complexity    32104    32118      +14     
   ============================================
     Files          1868     1866       -2     
     Lines        139000   138974      -26     
     Branches      15292    15291       -1     
   ============================================
   - Hits         101675   101654      -21     
   - Misses        29269    29279      +10     
   + Partials       8056     8041      -15     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | inttests | `24.20% <0.00%> (-0.03%)` | :arrow_down: |
   | systests | `25.00% <0.00%> (-0.13%)` | :arrow_down: |
   | unittests | `72.41% <0.00%> (+0.01%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://app.codecov.io/gh/apache/pulsar/pull/20611?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...hentication/oidc/AuthenticationProviderOpenID.java](https://app.codecov.io/gh/apache/pulsar/pull/20611?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci1hdXRoLW9pZGMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9icm9rZXIvYXV0aGVudGljYXRpb24vb2lkYy9BdXRoZW50aWNhdGlvblByb3ZpZGVyT3BlbklELmphdmE=) | `75.86% <0.00%> (ø)` | |
   | [...n/java/org/apache/pulsar/broker/PulsarService.java](https://app.codecov.io/gh/apache/pulsar/pull/20611?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9QdWxzYXJTZXJ2aWNlLmphdmE=) | `81.93% <ø> (-0.18%)` | :arrow_down: |
   | [...pache/pulsar/proxy/server/ProxyServiceStarter.java](https://app.codecov.io/gh/apache/pulsar/pull/20611?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLXByb3h5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvcHJveHkvc2VydmVyL1Byb3h5U2VydmljZVN0YXJ0ZXIuamF2YQ==) | `64.42% <ø> (-0.71%)` | :arrow_down: |
   | [...sar/websocket/service/WebSocketServiceStarter.java](https://app.codecov.io/gh/apache/pulsar/pull/20611?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLXdlYnNvY2tldC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL3dlYnNvY2tldC9zZXJ2aWNlL1dlYlNvY2tldFNlcnZpY2VTdGFydGVyLmphdmE=) | `73.91% <ø> (-1.09%)` | :arrow_down: |
   
   ... and [81 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/20611/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece merged pull request #20611: [cleanup][test] Remove and prevent junit

Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece merged PR #20611:
URL: https://github.com/apache/pulsar/pull/20611


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] tisonkun commented on a diff in pull request #20611: [cleanup][test] Remove and prevent junit

Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #20611:
URL: https://github.com/apache/pulsar/pull/20611#discussion_r1235696912


##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/extensions/data/BrokerLookupDataTest.java:
##########
@@ -44,21 +45,21 @@ public void testConstructors() {
                 webServiceUrl, webServiceUrlTls, pulsarServiceUrl,
                 pulsarServiceUrlTls, advertisedListeners, protocols, true, true,
                 ExtensibleLoadManagerImpl.class.getName(), System.currentTimeMillis(),"3.0");
-        Assert.assertEquals(webServiceUrl, lookupData.webServiceUrl());
-        Assert.assertEquals(webServiceUrlTls, lookupData.webServiceUrlTls());
-        Assert.assertEquals(pulsarServiceUrl, lookupData.pulsarServiceUrl());
-        Assert.assertEquals(pulsarServiceUrlTls, lookupData.pulsarServiceUrlTls());
-        Assert.assertEquals(Optional.of("9092"), lookupData.getProtocol("kafka"));
-        Assert.assertEquals(Optional.empty(), lookupData.getProtocol("echo"));
-        Assert.assertTrue(lookupData.persistentTopicsEnabled());
-        Assert.assertTrue(lookupData.nonPersistentTopicsEnabled());
-        Assert.assertEquals("3.0", lookupData.brokerVersion());
+        assertEquals(webServiceUrl, lookupData.webServiceUrl());
+        assertEquals(webServiceUrlTls, lookupData.webServiceUrlTls());

Review Comment:
   The same question should be asked and answered above.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] nodece commented on pull request #20611: [cleanup][test] Remove and prevent junit

Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #20611:
URL: https://github.com/apache/pulsar/pull/20611#issuecomment-1599983876

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org