Functional tests in golang #518

procrypt · 2017-03-27T19:26:59Z

This is the work I had done last week for writing functional test in golang. Here I am providing the output.json file to ioutil.ReadFile and then comparing it with the stdout of kompose convert.

In context to #432
@kadel @cdrage @containscafeine Let me know if this is the correct way to proceed.

cdrage · 2017-03-29T20:27:57Z

@procrypt

This looks great to me! 👍 Awesome work!

Just missing ExpectFailure right?

procrypt · 2017-04-03T07:40:04Z

@cdrage Thanks. I'll finish it soon ☺️

procrypt · 2017-04-17T04:48:52Z

@cdrage Please review.

cdrage · 2017-04-18T13:26:52Z

script/test/cmd/tests.go

+func main() {
+	ExpectSuccessK8s()
+	ExpectSuccessOpenShift()
+        ExpectSuccessAndWarningK8s()


gofmt issue?

hence travis failing

cdrage · 2017-04-26T14:19:24Z

Travis still failing / not working :(

script/test/cmd/tests.go:121: not enough arguments in call to ExpectSuccessK8s
script/test/cmd/tests.go:122: not enough arguments in call to ExpectSuccessOpenShift
script/test/cmd/tests.go:123: not enough arguments in call to ExpectSuccessAndWarningK8s
script/test/cmd/tests.go:124: not enough arguments in call to ExpectSuccessAndWarningOpenShift
script/test/cmd/tests.go:125: not enough arguments in call to ExpectFailureK8s
script/test/cmd/tests.go:126: not enough arguments in call to ExpectFailureOpenShift

ping @procrypt

concaf · 2017-04-26T14:20:55Z

script/test/cmd/tests.go

+	"strings"
+)
+
+func ExpectSuccessK8s(dockerComposeFile, result string) {


How about prefixing the test cases with Test?

concaf · 2017-04-26T14:22:16Z

script/test/cmd/tests.go

+	ExpectSuccessAndWarningOpenShift()
+	ExpectFailureK8s()
+	ExpectFailureOpenShift()
+}


I'd highly suggest doing this using the testing library. Please go through - https://golang.org/pkg/testing/

concaf

@procrypt nice work. Wanted to know what is the scope of these tests, have you moved all the kompose convert tests to golang, or just a few?

concaf · 2017-04-26T14:24:31Z

script/test/cmd/tests.go

+	ExpectSuccessAndWarningOpenShift()
+	ExpectFailureK8s()
+	ExpectFailureOpenShift()
+}


It would also be great if you could do it using the golang subtests way, please read about it, and you could also use this as a reference - https://github.com/redhat-developer/opencompose/blob/master/pkg/encoding/v1/encoding_test.go
Check for the use of tt.Run() in this file.

@containscafeine These tests are just comparing the output of kompose convert, so I recommended we use these tests like the way they are.

procrypt · 2017-05-02T07:24:29Z

@containscafeine I have moved all kompose convert test to golang.

procrypt · 2017-05-08T09:22:38Z

Ping @cdrage @surajssd

cdrage · 2017-05-08T13:49:07Z

@procrypt Tests still failing :(

procrypt · 2017-06-01T06:45:44Z

@cdrage Done.

concaf

Instead of defining so many functions for every kompose command, it'd be great if we use one function signature to encompass all/most of the kompose CLI options, WDYT?

concaf · 2017-06-01T14:22:38Z

script/test/cmd/cmd_test.go

+	if err != nil || fileNotExist(cmd) {
+		home := os.Getenv("HOME")
+		if home == "" {
+			fmt.Println("No $HOME environment variable found")


@procrypt did you make any changes to this? The error is still not getting handled, what am I missing?

concaf · 2017-06-01T14:23:29Z

script/test/cmd/cmd_test.go

+	cmdArgs := []string{"rev-parse", "--abbrev-ref", "HEAD"}
+	cmdOut, err := exec.Command(cmdName, cmdArgs...).CombinedOutput()
+	if err != nil {
+		fmt.Println("")


Why are we printing here?

concaf · 2017-06-01T14:29:48Z

script/test/cmd/cmd_test.go

+}
+
+//UpdateUri Replaces variables with current branch and uri
+func UpdateURI(uri, branch, inputfile, output string) error {


So, currently this function is tied to replacing only uri and branch, what do you think about making this more generic?
So, as an input we accept a map[string]string, in which pass key value pairs that we want to replace, which in this case will be, "%URI%": uri, and "%REF", branch, and then we iterate over the map and replace all the occurrences; this way we can replace any parameter from now.

concaf · 2017-06-01T14:30:11Z

script/test/cmd/cmd_test.go

+	cmdArgs := []string{"remote", "get-url", "origin"}
+	cmdOut, err := exec.Command(cmdName, cmdArgs...).CombinedOutput()
+	if err != nil {
+		fmt.Println("")


Why are we printing here?

concaf · 2017-06-01T14:31:04Z

script/test/cmd/cmd_test.go

+	if err != nil {
+		return errors.Wrapf(err, "Error in %s application", file[1])
+	}
+	er := ioutil.WriteFile(outFile, out, 0644)


Let's do err here, and reuse the err variable already created without using :=

concaf · 2017-06-01T14:31:47Z

script/test/cmd/cmd_test.go

+	cmdArgs := []string{command, "--provider", provider, "-f", dockerComposeFile, "--stdout", "-j"}
+	out, err := exec.Command(cmdName, cmdArgs...).CombinedOutput()
+	if err != nil {
+		fmt.Println("\x1b[31;1m===> Test Fail <===\x1b[0m")


"Test Failed"

concaf · 2017-06-01T14:33:48Z

script/test/cmd/cmd_test.go

+}
+
+// Kompose command
+func Kompose(command, provider, dockerComposeFile, result string) (cmd, output string, error error) {


What do you think about returning and accepting pointers to string instead of values of string?

Same for all the functions

concaf · 2017-06-01T14:36:47Z

script/test/cmd/cmd_test.go

+}
+
+// Kompose command for Multiple files
+func MultipleFiles(command, provider, dockerComposeFileOne, dockerComposeFiletwo, result string) (cmd, output string, error error) {


There is a lot of repeated code here; so, if I understand correctly, this function is the same as the function above, but accepts 2 docker compose files.
So, how about removing this and refactoring the function above to accept a []string as docker compose file and iterate on it. This way we can pass one or as many docker compose files that we want, without having to define the function again.

[]string will not work here as we provide multiple compose files as f <file1> -f <file2> not as -f <file1> <file2>

concaf · 2017-06-01T14:38:47Z

script/test/cmd/cmd_test.go

+}
+
+// Kompose command for bundle
+func KomposeBundle(command, dockerComposeFile, result string) (cmd, output string, error error) {


Even for this, we should have one Kompose function to which we should be able to pass which kompose subcommand to run, and if we pass bundle there, then the --bundle flag should get substituted accordingly.
WDYT?

concaf · 2017-06-01T14:39:00Z

script/test/cmd/cmd_test.go

+	return cmd, output, nil
+}
+
+//GenerateArtifacts generates artifacts in file with -o command


same as above

…al tests in golang kubernetes#518 to fail

procrypt · 2017-06-07T14:31:31Z

@containscafeine review please.

…al tests in golang kubernetes#518 to fail

added functional in golang for the test cases present in script/test/cmd/test.sh

moved fixtures from script/test to script/test/cmd because according to the go best pactices fixtures should be in the same directory where the test files are

kadel · 2017-06-12T09:52:35Z

Interesting, If I run make test-cmd couple times in the row, somethings "BuildConfig" test fails

procrypt · 2017-06-12T10:27:29Z

@kadel this is because of the environment variables, PR #631 will fix this.

…al tests in golang kubernetes#518 to fail

kadel · 2017-06-12T11:23:59Z

@kadel this is because of the environment variables, PR #631 will fix this.

But it's affecting only BuildConfig and nothing else

kadel

This is just a first part, haven looked at everything yet ;-)

kadel · 2017-06-12T11:29:19Z

script/test/cmd/cmd_test.go

+	if err != nil || fileNotExist(cmd) {
+		home := os.Getenv("HOME")
+		if home == "" {
+			fmt.Println("No $HOME environment variable found")


Why is $HOME tested?
Haven't found anything that would depend on $HOME

kadel · 2017-06-12T11:31:39Z

script/test/cmd/cmd_test.go

+	"testing"
+)
+
+func fileNotExist(path string) bool {


why this function even exists?
it just call sand verifies findExectuable, that function could be called directly

kadel · 2017-06-12T11:34:46Z

script/test/cmd/cmd_test.go

+	if m := d.Mode(); !m.IsDir() {
+		return nil
+	}
+	return os.ErrPermission


This function is called findExecutable but all it does is it verifies that file is not a dir.
And if its dir it returns osErrPermission, why?

kadel · 2017-06-12T11:43:17Z

script/test/cmd/cmd_test.go

+}
+
+//Git get the branch
+func Git() (branch string, error error) {


We already have function getGitCurrentBranch in openshift.go that is doing a similar thing.
It would be better to reuse that. (may require small refactoring and should be handled in separate PR). We could put all git related stuff into new 'git' package.

kadel · 2017-06-12T11:59:33Z

script/test/cmd/cmd_test.go

+}
+
+//Get origin
+func GitOrigin() (origin, branch string, error error) {


again, this might be similar to "getGitCurrentRemoteURL" in openshift.go

kadel · 2017-06-12T12:01:59Z

script/test/cmd/cmd_test.go

+}
+
+// Kompose command for buildContext
+func KomposeCommand(command, provider, outFile, dockerComposeFile string) (error error) {


this name is ultra confusing :-)

kadel · 2017-06-12T12:03:26Z

script/test/cmd/cmd_test.go

+}
+
+//Exec runs the kompose command
+func Exec(cmdName string, cmdArgs, dockerComposeFiles []string) (cmd string, err error) {


i don't think this function is even needed

…al tests in golang kubernetes#518 to fail

fejta-bot · 2017-12-27T11:37:15Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

fejta-bot · 2018-01-26T11:45:16Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

fejta-bot · 2018-02-25T12:30:49Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 27, 2017

procrypt force-pushed the functional_tests branch from 620b50d to 94a1180 Compare March 27, 2017 19:31

procrypt force-pushed the functional_tests branch 3 times, most recently from 82aedf2 to 96e5705 Compare April 17, 2017 04:48

cdrage requested changes Apr 18, 2017

View reviewed changes

procrypt force-pushed the functional_tests branch from 96e5705 to c03b74b Compare April 18, 2017 13:39

concaf reviewed Apr 26, 2017

View reviewed changes

concaf suggested changes Apr 26, 2017

View reviewed changes

concaf reviewed Apr 26, 2017

View reviewed changes

procrypt mentioned this pull request May 2, 2017

sort output when creating kubernetes/openshift objects #565

Merged

procrypt force-pushed the functional_tests branch from c03b74b to 55e9117 Compare May 8, 2017 09:20

procrypt force-pushed the functional_tests branch 4 times, most recently from f973b17 to 3e16596 Compare May 8, 2017 13:16

procrypt force-pushed the functional_tests branch 6 times, most recently from a50c24f to a06e5ac Compare May 9, 2017 06:01

procrypt force-pushed the functional_tests branch 5 times, most recently from 1b11ec1 to 6b44f13 Compare June 1, 2017 06:45

concaf suggested changes Jun 1, 2017

View reviewed changes

procrypt added a commit to procrypt/kompose that referenced this pull request Jun 6, 2017

Environment variabled are populated in random order, causing function…

54369f4

…al tests in golang kubernetes#518 to fail

procrypt mentioned this pull request Jun 6, 2017

Fix EnvSort struct #631

Merged

procrypt force-pushed the functional_tests branch from 6b44f13 to 65e4247 Compare June 7, 2017 14:29

procrypt force-pushed the functional_tests branch from 65e4247 to 43c8be9 Compare June 12, 2017 06:02

procrypt added a commit to procrypt/kompose that referenced this pull request Jun 12, 2017

Environment variabled are populated in random order, causing function…

b89eb54

…al tests in golang kubernetes#518 to fail

procrypt added 2 commits June 12, 2017 14:37

Addded functional test written in golang

036008b

added functional in golang for the test cases present in script/test/cmd/test.sh

Moved fixtures from script/test to script/test/cmd

f06c414

moved fixtures from script/test to script/test/cmd because according to the go best pactices fixtures should be in the same directory where the test files are

procrypt force-pushed the functional_tests branch from 43c8be9 to f06c414 Compare June 12, 2017 10:09

procrypt added a commit to procrypt/kompose that referenced this pull request Jun 12, 2017

Environment variabled are populated in random order, causing function…

13bb581

…al tests in golang kubernetes#518 to fail

kadel requested changes Jun 12, 2017

View reviewed changes

procrypt added a commit to procrypt/kompose that referenced this pull request Jun 14, 2017

Environment variabled are populated in random order, causing function…

719dae9

…al tests in golang kubernetes#518 to fail

procrypt mentioned this pull request Jun 19, 2017

move git and related functions from openshift.go into a separate file #651

Merged

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 27, 2017

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 26, 2018

k8s-ci-robot closed this Feb 25, 2018

Functional tests in golang #518

Functional tests in golang #518

Conversation

procrypt commented Mar 27, 2017 • edited

cdrage commented Mar 29, 2017

procrypt commented Apr 3, 2017

procrypt commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concaf left a comment

Choose a reason for hiding this comment

concaf Apr 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

procrypt commented May 2, 2017 • edited

procrypt commented May 8, 2017

cdrage commented May 8, 2017

procrypt commented Jun 1, 2017

concaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

procrypt Jun 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

procrypt commented Jun 7, 2017

kadel commented Jun 12, 2017

procrypt commented Jun 12, 2017

kadel commented Jun 12, 2017

kadel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta-bot commented Dec 27, 2017

fejta-bot commented Jan 26, 2018

fejta-bot commented Feb 25, 2018

procrypt commented Mar 27, 2017 •

edited

concaf Apr 26, 2017 •

edited

procrypt commented May 2, 2017 •

edited

procrypt Jun 5, 2017 •

edited