pass only required parameter to functions

[surajssd]

Lot of functions are being passed the entire app struct so refactoring, it to accept only what is needed.

Fixes #113

[kedge-bot]

@surajssd, thank you for the pull request! We'll ping some people to review your PR. @containscafeine, please review this.

[kedge-bot]

@surajssd, thank you for the pull request! We'll ping some people to review your PR. @containscafeine, please review this.

[kadel]

Why?
I don't like this to be honest. I think that in a lot of cases it makes sense to pass whole app struct.
You are always passing at least two arguments that are always in the same base struct why not passing the whole struct?

[surajssd]

@kadel because that struct has lot of other things that are not needed, so passing the function only things that it needs. why pass extra unnecessary things?

What is the rationale behind passing things to function that it does not need or might manipulate?

[surajssd]

@kadel also with this PR it fixes #113

see code: https://github.com/kedgeproject/kedge/pull/214/files#diff-e6ad971f18906f2176a088cf3bb5b070R54

[kadel]

@kadel because that struct has lot of other things that are not needed, so passing the function only things that it needs. why pass extra unnecessary things?

but you are passing arguments from the same struct. How does it make sense to break it to multiple arguments?

[kadel]

What is @containscafeine's opinion on this?

If he agrees with this lets merge it. I'm OK with that.

[surajssd]

ping @containscafeine

[concaf]

I'm confused, why are the tests like this? Why not put all the tests in one tests struct which has a field succeed: bool

[surajssd]

that way the testing code becomes really hard to understand with all the conditions around whether the test is expected to pass or fail, this is mere separation of successful tests and failing tests.

[concaf]

How does it become difficult to understand, there is a field right there next to the test which says if the test is expected to pass or fail.
Also, if we separate passing and failing tests, we are sorting by test behavior not by the functionality that is being tested. One behavior will have passing and failing tests defined separately and will be very difficult to track the tests for a given behavior.

If we absolutely have to make things concise, we can have different []struct for different behavior.

[surajssd]

How does it become difficult to understand, there is a field right there next to the test which says if the test is expected to pass or fail.

The part that becomes difficult to understand is the code that runs test, for example:

	for _, test := range tests {
		t.Run(test.Name, func(t *testing.T) {
			kd, err := transformer.CreateDeployments(test.Service)
			if err != nil {
				if test.Succeed {
					t.Errorf("Failed to create deployment from %#v\nErr: %s", test.Service, err)
				}
				return
			}


			if !test.Succeed {
				t.Errorf("Expected failure, but succeeded, service: %#v\nConverted k8s Deployment: %#v", test.Service, spew.Sprint(kd))
				return
			}


			if !reflect.DeepEqual(kd, test.K8sDeployments) {
				t.Errorf("Expected: %#v\nGot: %#v\n", spew.Sprint(test.K8sDeployments), spew.Sprint(kd))
				return
			}
		})
	}

Taken from opencompose in here

If you are checking condition on test.Succeed which is used twice, it goes out of the window when we have separated those two things.

And see the code we have here:

	for _, test := range passingTests {
		t.Logf("Running test: %s", test.Name)
		got, err := fixServices(test.Input, appName)
		if err != nil {
			t.Errorf("expected to pass but failed with: %v", err)
		}
		if !reflect.DeepEqual(got, test.Output) {
			t.Errorf("expected: %s, got: %s", prettyPrintObjects(test.Output),
				prettyPrintObjects(got))
		}
	}

source here

No convoluted logic of test.Succeed here. Because we know test should pass and all output should match as well.

And for failing tests we just need to see if it failed:

	got, err := fixVolumeClaims(passingTest, appName)
	if err != nil {
		t.Errorf("expected to pass but failed with: %v", err)
	}
	if !reflect.DeepEqual(got, expected) {
		t.Errorf("expected: %s, got: %s", prettyPrintObjects(expected),
			prettyPrintObjects(got))
	}

source here

[concaf]

I think we can sort that out by adding comments, I don't think we need to have different passing and failing structs. It will be very hard to track passing and failing tests for a given behavior.
ping @kadel @cdrage WDYT?

[surajssd]

@surajssd okay, suppose I want to test the behavior of fixing names, then I'd logically want to put the failing and passing tests together instead of splitting in different functions.

What does test the behavior of fixing names mean ?

[surajssd]

Let's not bike shed on this and get something done, I think!

[concaf]

I'm sorry, I'm nack on this.
Separating tests based on passing or failing makes it impossible to track the behavior being tested. We already have a structure in place, I don't understand why are we deviating from that.
I'm sorry if you feel this is bikeshedding, but this PR is deviating from the way things have been getting done for no good reason.
ping @kadel @cdrage

[surajssd]

Please see the code from Kubernetes again, the tests are separated on passing and failing behavior.

Passing tests, where output is matched: here

Failing tests, where you will certainly get an error and you are not matching anything just checking if the function being tested errored out: here

[kadel]

Isn't this the same discussion as in #208 (comment) ?
Can we solve this in one place?

[pradeepto]

Let's move ahead on this bit and merge it unless there is huge problem. We can come back to it if need be.