Feature/protected branch and example test properties #156

gmessner · 2018-03-12T05:21:15Z

Added bean test for ProtectedBranch in TestGitLabApiBeans and fixed issues exposed by the bean test (AccessLevel was a String, not an Integer).
Added all known properties to the example-test-gitlab4j.properties file with comments and added logic to the test classes to skip tests that do not have the required property values set.

lamdav

@gmessner
Just 3 suggestions:

Consider stubbing out the example test properties similar to above fields or removing stubs in general to maintain consistency.
Consider using isEmpty() over manually checking string length > 0.
ProtectedBranch test may now have unintended results.

lamdav · 2018-03-12T16:28:14Z

example-test-gitlab4j.properties

+# To test using GitLab4J-API with a proxy, set the following properties
+TEST_PROXY_URI=
+TEST_PROXY_USERNAME=
+TEST_PROXY_PASSWORD=


Should we follow the above convention of having some stub string such as some-proxy-password?

As you imply, to be consistent it should be one way or the other. Providing a value could lead to some confusion, (are these valid values?), so I suggest we remove all values, something like this:

# # To test GitLab4J-API, you must have access to a GitLab server. # # Copy this file to test-gitlab4j.properties and set the property # values to match the GitLab server you are testing against. # # REQUIRED: The following values must be set or the tests will fail. # The TEST_PRIVATE_TOKEN and TEST_USERNAME must belong to an admin user. TEST_PRIVATE_TOKEN= TEST_ACCESS_TOKEN= TEST_USERNAME= TEST_NAMESPACE= TEST_PROJECT_NAME= TEST_HOST_URL= # OPTIONAL: To test the GroupApi, set these to an existing group name and group-project name, # and the username of a group member TEST_GROUP= TEST_GROUP_PROJECT= TEST_GROUP_MEMBER_USERNAME= # OPTIONAL: To test oauth2Login, set these properties TEST_LOGIN_USERNAME= TEST_LOGIN_PASSWORD= # OPTIONAL: To test sudo capability provide a username to sudo as TEST_SUDO_AS_USERNAME= # OPTIONAL: To test using GitLab4J-API with a proxy, set the following properties TEST_PROXY_URI= TEST_PROXY_USERNAME= TEST_PROXY_PASSWORD=

I'll push the above unless you have a very reason to do it differently.

I agree with what you said about validity. My comment was more so on picking one convention over another.

I figured that, I just pushed this change.

lamdav · 2018-03-12T16:30:33Z

src/test/java/org/gitlab4j/api/TestGitLabApi.java

@@ -77,6 +77,7 @@ public void testGetVersion() throws GitLabApiException {
    @Test
    public void testProxyConnection() throws GitLabApiException {
        assumeTrue(TEST_PROXY_URI != null && TEST_PROXY_USERNAME != null && TEST_PROXY_PASSWORD != null);
+        assumeTrue(TEST_PROXY_URI.length() > 0 && TEST_PROXY_USERNAME.length() > 0 && TEST_PROXY_PASSWORD.length() > 0);


Suggest using TEST_PROXY_URI.isEmpty() for length checks.

TEST_PROXY_URI.length() > 0
vs
!TEST_PROXY_URI.isEmpty()

When checking for truth, I prefer to use a statement that results in true, not something that has to be negated to produce true. It's just a personal preference and is used throughout GitLab4J, so to change it in these instances would require a sweep through all the code to be consistent.

The reasoning sounds good to me.

@lamdav
FYI, when checking for empty instead of not empty I should be using isEmpty(), so I have gone through all the tests and replaced str.size() == 0 with str.isEmpty(), just thought I would let you know.

lamdav · 2018-03-12T16:38:04Z

src/test/java/org/gitlab4j/api/TestGroupApi.java

-                testUser = gitLabApi.getUserApi().getUser(TEST_GROUP_MEMBER_USERNAME);
-            } catch (GitLabApiException gle) {
-                problems += "Problem fetching test user, error=" + gle.getMessage() + "\n";
+            if (TEST_GROUP_MEMBER_USERNAME != null && TEST_GROUP_MEMBER_USERNAME.length() > 0) {


if (TEST_GROUP_MEMBER_USERNAME != null && !TEST_GROUP_MEMBER_USERNAME.isEmpty()) { ... }

See my response above

lamdav · 2018-03-12T16:42:25Z

src/test/java/org/gitlab4j/api/TestProtectedBranchesApi.java

@@ -157,6 +156,6 @@ public void testProtectBranch() throws GitLabApiException {
        List<ProtectedBranch> branches = gitLabApi.getProtectedBranchesApi().getProtectedBranches(project.getId());
        assertNotNull(branches);
        assertTrue(branches.stream()
-                .anyMatch((protectedBranch) -> protectedBranch.getName().equals(TEST_BRANCH_NAME)));
+                .anyMatch((protectedBranch) -> branch.getName().equals(TEST_BRANCH_NAME)));


Correct me if I am wrong, but I think this should remain unchanged. I wrote this test to protect an unprotected branch, get a list of protected branches, and verify that the unprotected branch is now part of this list.

By changing the name, the assertion will always return true as branch is effectively treated as a final variable (line 155) and should always be TEST_BRANCH_NAME by the time (line 159) executes. If it does not, (line 155) should have failed with an exception thrown.

Not sure how that got changed, fixed and pushed.

Made another change to this. After the revert to protectedBranch the branch variable was not referenced, and I realized that the contents of the returned protected branch were never really validated, by validating the name, it took care of the unused reference.

lamdav · 2018-03-12T16:44:57Z

src/test/java/org/gitlab4j/api/TestUserApi.java

@@ -149,7 +153,7 @@ public void testGetOptionalUser() throws GitLabApiException {
    @Test
    public void testSudoAsUser() throws GitLabApiException {

-        assumeTrue(TEST_SUDO_AS_USERNAME != null);
+        assumeTrue(TEST_SUDO_AS_USERNAME != null && TEST_SUDO_AS_USERNAME.length() > 0);


See my response above.

lamdav · 2018-03-12T18:02:41Z

@gmessner
LGTM

gmessner · 2018-03-12T18:07:32Z

Thanks for all your feedback on this, and the initial contribution.

BTW, I'll get to the RunnerApi submission when I get home from my day job tonight.

gmessner added 3 commits March 11, 2018 22:09

Initial commit.

c2186d4

Minor mods to protected branch functionality.

44eb844

Cleaned up example-test-gitlab4j.properties and releated tests.

d4906b7

gmessner force-pushed the feature/protected-branch-and-example-test-properties branch from 3eafed8 to d4906b7 Compare March 12, 2018 06:59

gmessner mentioned this pull request Mar 12, 2018

Project Branches API #153

Merged

lamdav reviewed Mar 12, 2018

View reviewed changes

gmessner added 2 commits March 12, 2018 10:36

Fixed test that was accidentally broken.

1db1f77

Now verifies branch name of returned protected branch.

cac5f11

Improved consistency and made comments clearer.

5847870

gmessner merged commit 2267922 into master Mar 12, 2018

gmessner deleted the feature/protected-branch-and-example-test-properties branch March 12, 2018 19:23

Feature/protected branch and example test properties #156

Feature/protected branch and example test properties #156

Uh oh!

Conversation

gmessner commented Mar 12, 2018

Uh oh!

lamdav left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

lamdav commented Mar 12, 2018

Uh oh!

gmessner commented Mar 12, 2018

Uh oh!

Uh oh!

lamdav left a comment •

edited

Loading