Add unit tests for Utils #4193

joshknopp · 2022-01-05T03:38:11Z

What's the purpose of this PR

Increase test coverage on one class to help push toward 80% coverage goal.

Which issue(s) this PR fixes:

Fixes #3874

Brief changelog

Add 100% test coverage for a single Utils class

Follow this checklist to help us incorporate your contribution quickly and easily:

Read the Contributing Guide before making this pull request.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit tests to verify the code.
Run mvn clean test to make sure this pull request doesn't break anything.
Update the CHANGES log.

github-actions · 2022-01-05T03:38:28Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

joshknopp · 2022-01-05T03:42:27Z

I have read the CLA Document and I hereby sign the CLA

joshknopp · 2022-01-05T12:30:27Z

Squashed commits, should now show as one commit.

Please note comment intended for @nobodyiam regarding case sensitivity.

codecov-commenter · 2022-01-05T12:53:50Z

Codecov Report

Merging #4193 (b7d4619) into master (02f0dca) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #4193   +/-   ##
=========================================
  Coverage     52.60%   52.60%           
  Complexity     2621     2621           
=========================================
  Files           484      484           
  Lines         15192    15192           
  Branches       1571     1571           
=========================================
  Hits           7991     7991           
  Misses         6645     6645           
  Partials        556      556

Impacted Files	Coverage Δ
...rk/apollo/spring/property/SpringValueRegistry.java	`83.78% <0.00%> (-5.41%)`	⬇️
...om/ctrip/framework/foundation/internals/Utils.java	`83.33% <0.00%> (+33.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02f0dca...b7d4619. Read the comment docs.

Anilople · 2022-01-05T12:55:15Z

Welcome~ @joshknopp

apollo-core/src/test/java/com/ctrip/framework/foundation/internals/UtilsTest.java

Anilople · 2022-01-05T12:57:17Z

apollo-core/src/test/java/com/ctrip/framework/foundation/internals/UtilsTest.java

+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * @author Josh Knopp(https://github.com/joshknopp)


Please delete the author in javadoc, because your change will be show in git's commit.

No problem - was trying to follow style guide, but I'm indifferent 😄

Anilople · 2022-01-05T13:02:42Z

apollo-core/src/test/java/com/ctrip/framework/foundation/internals/UtilsTest.java

+        private String actualOsName;
+
+        @BeforeEach
+        void setUp() {
+            actualOsName = System.getProperty("os.name");
+        }


How about delete the setUp method?
Just assign value to actualOsName once.

Suggested change

private String actualOsName;

@BeforeEach

void setUp() {

actualOsName = System.getProperty("os.name");

}

private final String actualOsName = System.getProperty("os.name");

Thanks @Anilople for the feedback! Updated in new commit.

joshknopp · 2022-01-05T14:41:52Z

Just realized I have added JUnit5 tests, and everything else appears to be JUnit4 🤦‍♂️ Is there appetite for 5, or should I reset and try again?

apollo-core/src/test/java/com/ctrip/framework/foundation/internals/UtilsTest.java

nobodyiam · 2022-01-06T00:55:37Z

Just realized I have added JUnit5 tests, and everything else appears to be JUnit4 🤦‍♂️ Is there appetite for 5, or should I reset and try again?

It may keep some sort of consistency if we write the unit tests in JUnit 4 styles but generally, I think it's all right. What do you think @Anilople ?

joshknopp · 2022-01-06T02:54:44Z

Just realized I have added JUnit5 tests, and everything else appears to be JUnit4 🤦‍♂️ Is there appetite for 5, or should I reset and try again?

It may keep some sort of consistency if we write the unit tests in JUnit 4 styles but generally, I think it's all right. What do you think @Anilople ?

Looks like Codecov is not picking up JUnit 5 so I'll try rewriting with 4.

hezhangjian · 2022-01-06T05:38:58Z

@nobodyiam com.ctrip.framework.apollo.core.utils.StringUtils has a method isBlank too.
What's the relationship between com.ctrip.framework.foundation and com.ctrip.framework.apollo.
Maybe we only need one #isBlank method

hezhangjian · 2022-01-06T06:07:05Z

@nobodyiam do we consider use apache common-langs ?

Anilople · 2022-01-06T11:41:33Z

@nobodyiam do we consider use apache common-langs ?

A reliable middleware need to reduce dependencies as it can. So if we can implement the function via a few code, it's better implement without third dependencies.

Just a bit of my thoughts.

Anilople · 2022-01-06T11:51:18Z

Just realized I have added JUnit5 tests, and everything else appears to be JUnit4 🤦‍♂️ Is there appetite for 5, or should I reset and try again?

It may keep some sort of consistency if we write the unit tests in JUnit 4 styles but generally, I think it's all right. What do you think @Anilople ?

I think it's all right too in this pr.

An issue #4195 was opend to disscus upgrade to junit 5 or not.

hezhangjian · 2022-01-06T12:10:40Z

@Anilople commons-lang is stable, Anyway and at least, I don't think that we need two isBlank method in com.ctrip.framework.foundation and com.ctrip.framework.apollo

nobodyiam

LGTM

nobodyiam · 2022-01-07T00:51:38Z

@Anilople commons-lang is stable, Anyway and at least, I don't think that we need two isBlank method in com.ctrip.framework.foundation and com.ctrip.framework.apollo

@shoothzj
I checked the commit history and found these 2 isBlank methods were introduced in different historic reasons. But I agree with you it's unnecessary to have 2 isBlank methods with the same purpose. I think we could open another pr to fix this and keep this pr only to serve the unit test purpose.
As for commons-lang, I agree with @Anilople that it's generally a good practice to keep the sdk's dependencies as less as possible, e.g. apollo-core now only has 3 compile scope dependencies (gson, guava, slf4j-api). So unless it's really necessary, I would suggest we keep the custom version of StringUtils.

hezhangjian · 2022-01-07T01:18:00Z

Agreed. I will start another issue discuss this

joshknopp force-pushed the add-tests-for-utils branch from 85f9758 to 6f4b525 Compare January 5, 2022 12:28

Anilople reviewed Jan 5, 2022

View reviewed changes

apollo-core/src/test/java/com/ctrip/framework/foundation/internals/UtilsTest.java Show resolved Hide resolved

Anilople reviewed Jan 5, 2022

View reviewed changes

joshknopp changed the title ~~Add tests for utils~~ Add unit tests for Utils Jan 5, 2022

joshknopp force-pushed the add-tests-for-utils branch 2 times, most recently from 0d142d3 to 1b87ff7 Compare January 5, 2022 13:59

joshknopp requested a review from Anilople January 5, 2022 20:47

nobodyiam reviewed Jan 6, 2022

View reviewed changes

Add test coverage for Utils

b7d4619

joshknopp force-pushed the add-tests-for-utils branch from 1b87ff7 to b7d4619 Compare January 6, 2022 03:18

joshknopp requested a review from nobodyiam January 6, 2022 03:19

Anilople mentioned this pull request Jan 6, 2022

upgrade junit to 5 or not? #4195

Open

Anilople added the area/test Categorizes issue or PR as related to testing label Jan 6, 2022

nobodyiam approved these changes Jan 7, 2022

View reviewed changes

hezhangjian approved these changes Jan 7, 2022

View reviewed changes

Anilople merged commit fec79aa into apolloconfig:master Jan 7, 2022

github-actions bot locked and limited conversation to collaborators Jan 7, 2022

nobodyiam added this to the 2.0.0 milestone Jan 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests for Utils #4193

Add unit tests for Utils #4193

joshknopp commented Jan 5, 2022 •

edited

Loading

github-actions bot commented Jan 5, 2022 •

edited

Loading

joshknopp commented Jan 5, 2022

joshknopp commented Jan 5, 2022

codecov-commenter commented Jan 5, 2022 •

edited

Loading

Anilople commented Jan 5, 2022

Anilople Jan 5, 2022

joshknopp Jan 5, 2022

Anilople Jan 5, 2022

joshknopp Jan 5, 2022 •

edited

Loading

joshknopp commented Jan 5, 2022

nobodyiam commented Jan 6, 2022

joshknopp commented Jan 6, 2022

hezhangjian commented Jan 6, 2022

hezhangjian commented Jan 6, 2022

Anilople commented Jan 6, 2022

Anilople commented Jan 6, 2022

hezhangjian commented Jan 6, 2022 •

edited

Loading

nobodyiam left a comment

nobodyiam commented Jan 7, 2022

hezhangjian commented Jan 7, 2022

Add unit tests for Utils #4193

Add unit tests for Utils #4193

Conversation

joshknopp commented Jan 5, 2022 • edited Loading

What's the purpose of this PR

Which issue(s) this PR fixes:

Brief changelog

github-actions bot commented Jan 5, 2022 • edited Loading

joshknopp commented Jan 5, 2022

joshknopp commented Jan 5, 2022

codecov-commenter commented Jan 5, 2022 • edited Loading

Codecov Report

Anilople commented Jan 5, 2022

Anilople Jan 5, 2022

Choose a reason for hiding this comment

joshknopp Jan 5, 2022

Choose a reason for hiding this comment

Anilople Jan 5, 2022

Choose a reason for hiding this comment

joshknopp Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

joshknopp commented Jan 5, 2022

nobodyiam commented Jan 6, 2022

joshknopp commented Jan 6, 2022

hezhangjian commented Jan 6, 2022

hezhangjian commented Jan 6, 2022

Anilople commented Jan 6, 2022

Anilople commented Jan 6, 2022

hezhangjian commented Jan 6, 2022 • edited Loading

nobodyiam left a comment

Choose a reason for hiding this comment

nobodyiam commented Jan 7, 2022

hezhangjian commented Jan 7, 2022

joshknopp commented Jan 5, 2022 •

edited

Loading

github-actions bot commented Jan 5, 2022 •

edited

Loading

codecov-commenter commented Jan 5, 2022 •

edited

Loading

joshknopp Jan 5, 2022 •

edited

Loading

hezhangjian commented Jan 6, 2022 •

edited

Loading