feat: add unit tests #5

xoanmm · 2021-12-27T16:23:23Z

No description provided.

…git-server for tests

JorTurFer

In general, LGTM.
Only small nits

JorTurFer · 2022-01-03T08:17:45Z

.github/workflows/pull_request_tests.yml

+      - name: View all in port 22
+        run: |
+          sudo lsof -i -P -n | grep 22
+          sudo lsof -i:22
+
+      - name: Kill all in port 22
+        run: |
+          sudo fuser -k 22/tcp
+
+      - name: View all in port 22
+        run: |
+          sudo lsof -i -P -n | grep 22
+          sudo lsof -i:22


Why are these lines needed? In agent runs from scratch, and you shouldn't kill anything there :/
Maybe we should change the port that we use if it's in conflict with ssh server port

This was only a quick test, and really was a bad idea. Now all the actions related with tests have been modified in Makefile to start a container that will create a simple git server with a repo created from scratch and it will be exposed in port 2222.

Makefile

JorTurFer · 2022-01-03T08:31:48Z

launch_test_env.sh

+#!/bin/bash
+
+ACTION="${1}"
+
+if [[ "$ACTION" == "create" ]]; then
+    echo "It's going to launch actions for create tests environment"
+    # Create git-server
+    docker-compose -f test-git-server/docker-compose.yaml up -d --build git-server
+    # Create repo in git-server
+    docker-compose -f test-git-server/docker-compose.yaml up --build create-repo
+elif [[ "$ACTION" == "destroy" ]]; then
+    # Delete git-server and create-repo containers
+    docker-compose -f test-git-server/docker-compose.yaml up down
+else
+    echo "ACTION provided $ACTION is not a valid command. The only options available are 'create' and 'destroy'"
+fi


Should this be handled by make instead another script? I mean, something like make launch

You're right, updated the Makefile for launch as make launch-test-deps, this will be launched also as first step of any target of make related with tests

Co-authored-by: Jorge Turrado Ferrero <[email protected]>

patrykwozinski

Thanks @xoanmm! I reviewed .go files and left some minor questions/suggestions/comments. Feel free to ignore them if something is not valid, I'm not an expert of Go nor this context of the HelmRepoUpdater. No changes requested, only comments-like things.

patrykwozinski · 2022-01-10T12:53:22Z

internal/app/git/creds.go

 		if g.SSHPrivKey != "" {
-
 			return git.NewSSHCreds(g.SSHPrivKey, "", true), nil
+		} else {
+			return nil, fmt.Errorf(
+				"sshPrivKey not provided for authenticatication to repository %s",
+				repoURL,
+			)


In such cases, in my opinion, it's always better to avoid using the else statement. For example you can do:

if g.SSHPrivKey == "" { return nil, fmt.Errorf( "sshPrivKey not provided for authenticatication to repository %s", repoURL, ) } return git.NewSSHCreds(g.SSHPrivKey, "", true), nil

Sometimes it's called "early exit".

What do you think @xoanmm? It's not a change request, only a suggestion to consider.

AFAIK, else it's bad practice in golang and usually linters throw an error

Yes, you're right, correcting just now to update in next commit. Thanks :D

patrykwozinski · 2022-01-10T12:54:42Z

internal/app/git/creds.go

+		} else {
+			return nil, fmt.Errorf(
+				"no value provided for username and password for authentication to repository %s",
+				repoURL,
+			)


Same in the previous comment. Maybe it's worth considering if early exit is not fitting here instead of the else statement.

Yes, you're right, correcting just now to update in next commit. Thanks :D

patrykwozinski · 2022-01-10T13:00:40Z

internal/app/updater/commit.go


 }

 // commitChangesLocked commits the changes to the git repository.
-func commitChangesLocked(cfg HelmUpdaterConfig, state *SyncIterationState) error {
+func commitChangesLocked(cfg HelmUpdaterConfig, state *SyncIterationState) (*[]ChangeEntry, error) {


Can we return a value []ChangeEntry instead of the pointer here? It is possible, or it's nonsense? Sorry for the dumb question!

In this case because normally when are returned pairs like Struct, error and there is the possibility that error is not nil is returned the first object as nil and for that is need to declared as pointer, in this case really because the return of the function is a call to function commitChangesGit that need to be able to return nil as the first object

patrykwozinski · 2022-01-10T13:09:43Z

internal/app/git/creds_test.go

+	if !(reflect.DeepEqual(creds, expectedCreds)) {
+		log.Fatalf("creds and expectedCreds are equal: %t\n", reflect.DeepEqual(creds, expectedCreds))
+	}


Hmm, I'm wondering if it's not worth using stretchr/testify for such cases. I mean - making an inversed if statement is solid, but it's not that clear from my point of view for code-readers.

You can read more here.

Of course, it's not necessary, but that might somehow improve the readability of tests.

You're right, is better use an assert approach, I didn't know about stretchr/testify, for that type of tests normally I use library "gotest.tools/v3/assert", and this test now looks like the following:

func TestNewCredsSSHURLSShPrivKey(t *testing.T) { g := Credentials{ Email: validGitCredentialsEmail, SSHPrivKey: validGitCredentialsSSHKey, } repoURL := validGitRepoSSHURL creds, err := g.NewCreds(repoURL) if err != nil { log.Fatal(err) } expectedCreds := git.NewSSHCreds(validGitCredentialsSSHKey, "", true) assert.DeepEqual(t, creds, expectedCreds, cmp.AllowUnexported(git.SSHCreds{})) }

Nice, it's cleaner I think now! Thank you!

patrykwozinski · 2022-01-10T13:13:57Z

internal/app/git/creds_test.go

+		Password: validGitCredentialsPassword,
+	}
+
+	repoURL := validGitRepoHTTPSURL


Is this temp variable necessary? Maybe we can use validGitRepoHTTPSURL directly for NewCreeds? What do you think?

Yes, you're right, is not necessary declare a local variable for that, it can be used validGitRepoHTTPSURL for call NewCreds. Updating for upload in next commit

patrykwozinski · 2022-01-10T13:31:45Z

internal/app/git/creds.go

@@ -17,15 +18,23 @@ type Credentials struct {
 func (g Credentials) NewCreds(repoURL string) (git.Creds, error) {


Not related to this PR (!).

In Golang NewSomething is an approach to create a new instance of something (static factory method). For example, if you have a GitHubClient struct, and you want to create a new instance of it, then you should have a function called NewGitHubClient. In our case - this function is something for fetching credentials from the git library (whatever it is) so maybe it's better to rename it in the future to something more clear.

Maybe it will be a good idea change to NewGitCreds, wdyt?

patrykwozinski · 2022-01-10T13:43:05Z

internal/app/updater/commit_test.go

+const validHelmAppName = "example-app"
+const validHelmAppFileToChange = validHelmAppName + "/values.yaml"
+
+func getRouteRelativePath(numRelativePath int, relativePath string) (*string, error) {


It's really, really not an important comment/suggestion, but private functions IMO should be on the bottom of files. Thanks to that we'll hide the complexity, and the reviewer will not focus on it at the beginning, and that's fine because it's an implementation detail. :)

I agree, and in this case I'll move this function to a new package called utils, and also convert into public function, because it will be necessary add more and used it in different internal packages. wdyt?

patrykwozinski · 2022-01-10T13:45:51Z

internal/app/updater/commit_test.go

+func TestMain(m *testing.M) {
+	code := m.Run()
+	os.Exit(code)
+}


What's the purpose of this function? I mean, os.Exit(m.Run()) is not a default implementation of the TestMain function?

From my point of view (I'm so f***cking noob in Go) this should be reserved for bootstrapping more complex tests where you need to set up something before tests and it's repeatable for all test functions in it.

reference

Initially I thought it might be necessary to set up extra things but it has not been necessary, so I will remove this feature from all tests for the time being.

patrykwozinski · 2022-01-10T14:02:47Z

internal/app/updater/override.go

@@ -52,40 +52,39 @@ func overrideValues(cfg HelmUpdaterConfig, targetFile string) ([]ChangeEntry, er
 	for _, app := range cfg.UpdateApps {
 		// define new entry
 		var newEntry ChangeEntry
-		var oldValue, newValue string
+		var oldValue, newValue *string


Why do we need a pointer here?

patrykwozinski · 2022-01-10T14:09:23Z

internal/app/yq/commands_test.go

+	tmpFileName := tmpFile.Name()
+	err = ioutil.WriteFile(tmpFileName, yamlData, 0644)
+	if err != nil {
+		return nil, err
+	}


We don't need to close this file in such a case? Maybe it's worth doing a defer for it?

feat: add initial unit tests for yq package

0bafa1b

xoanmm requested a review from a team December 27, 2021 16:23

xoanmm self-assigned this Dec 27, 2021

xoanmm requested review from lupiuz, mknet3 and ishioni and removed request for a team December 27, 2021 16:23

xoanmm linked an issue Dec 27, 2021 that may be closed by this pull request

add unit tests and update Github Actions adding it #4

Closed

xoanmm added 11 commits December 27, 2021 17:29

feat: add Makefile and modify github actions to run unit tests

10c9c9d

feat: add unit tests for internal git package

6ebcf5a

feat: add tests for commit package and docker-compose for have local …

3ba68db

…git-server for tests

fix: add command kill all in 22 port

f345815

fix: correct lint problems

3641b08

fix: add command debug problem of ports

0812cfd

fix: change commands to debug

3f3e7f0

fix: add more commands to kill processes

09ea98c

fix: remove command with problems

f15244e

fix: add command to delete connections

35678bc

fix: change commands delete process

97f8f88

alv91 approved these changes Jan 3, 2022

View reviewed changes

JorTurFer suggested changes Jan 3, 2022

View reviewed changes

Update Makefile

ff0ffb2

Co-authored-by: Jorge Turrado Ferrero <[email protected]>

patrykwozinski reviewed Jan 10, 2022

View reviewed changes

xoanmm added 7 commits January 11, 2022 16:24

fix: add solution for test locally git push command

3cb3f1d

fix: remove innecesary lines

8fee982

fix: update Makefile

be0f05d

fix: update type of keys for tests

b2e0554

fix: add command for add ssh key

0c04b14

fix: add necessary change of permissions for private key

2cc4e33

fix: change way add ssh key to agent

fa1f216

xoanmm and others added 17 commits January 11, 2022 17:43

fix: change way add key

f12d766

fix: add reference to secret

d391421

fix: change tests github actions

b35e124

fix: test github actions

995093d

fix: change way retrieve ssh private key

94a7b2c

fix: add ssh private key with commands

7f9b415

fix: remove innecesary line

7c16789

fix: simplify way of configure SSH_PRIVATE_KEY

5713973

fix: change way add ssh-key

f303d7b

fix: add creation of directory

3979b3d

fix: add necessary startup of git-server

de8e762

fix: add cat for test purpose

3d84485

fix: add print for check

5727c78

fix: add commands for tests

28d4836

fix: add lines tocheck existence of files

b449945

fix: add checks

f4ec841

Refactor for fix SSH key loading

c2ee736

patrykwozinski approved these changes Jan 12, 2022

View reviewed changes

fix: remove innecesary file

0fc8265

xoanmm merged commit 6a62eb5 into develop Jan 12, 2022

xoanmm deleted the feature/create-unit-tests branch January 12, 2022 15:16

		@@ -17,15 +18,23 @@ type Credentials struct {
		func (g Credentials) NewCreds(repoURL string) (git.Creds, error) {

feat: add unit tests #5

feat: add unit tests #5

Uh oh!

Conversation

xoanmm commented Dec 27, 2021

Uh oh!

JorTurFer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xoanmm Jan 12, 2022 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrykwozinski 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!

xoanmm Jan 12, 2022 • 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!

xoanmm Jan 12, 2022 • 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!

Uh oh!

xoanmm Jan 12, 2022 •

edited

Loading

patrykwozinski left a comment •

edited

Loading

xoanmm Jan 12, 2022 •

edited

Loading

xoanmm Jan 12, 2022 •

edited

Loading