Skip to content
This repository has been archived by the owner on Nov 15, 2022. It is now read-only.

clean up test fixtures with test code #180

Merged
merged 1 commit into from
Jul 26, 2017

Conversation

surajssd
Copy link
Member

Right now the test fixtures are in different files and each test case data is named as opposed to putting in slice so removing all those files and created a slice of test data.

Fixes #69

@kedge-bot
Copy link
Collaborator

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

@kedge-bot kedge-bot requested a review from concaf July 25, 2017 11:59
@surajssd surajssd requested a review from kadel July 25, 2017 11:59
pkgcmd "github.com/kedgeproject/kedge/pkg/cmd"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this happened according to this #149 (comment)

@concaf
Copy link
Collaborator

concaf commented Jul 25, 2017

This makes one file very huge, and splitting to multiple files was done to reduce that so tests become more readable.

I'm +1 on refactoring this, but how about we do it in a way where we put every test case in one file.
e.g. this part will go in one file -

		{
			Name: "One container mentioned in the spec",
			Data: []byte(`
name: test
containers:
 - image: nginx
services:
  - ports:
    - port: 8080`),
			App: &spec.App{
				Name: "test",
				PodSpecMod: spec.PodSpecMod{
					Containers: []spec.Container{
						{
							Container: api_v1.Container{
								Image: "nginx",
							},
						},
					},
				},
				Services: []spec.ServiceSpecMod{
					{
						Name: "test",
						Ports: []spec.ServicePortMod{
							{
								ServicePort: api_v1.ServicePort{
									Port: 8080,
								},
							},
						},
					},
				},
			},
		},

and we will have multiple such files, one for every test case.

In OpenCompose, it became a pain to debug huge files, so I really do not want that :(

@surajssd
Copy link
Member Author

@containscafeine naming already is a problem and then naming each file will just add problems and unnecessary cognitive burden!

@concaf
Copy link
Collaborator

concaf commented Jul 25, 2017

@surajssd simple naming conventions like test1.go, test2.go can fix that. I really don't want to be debugging huge files.
-1 on putting everything in one giant file :(

Right now the test fixtures are in different files and
each test case data is named as opposed to putting in
slice so removing all those files and created a slice
of test data.
@surajssd
Copy link
Member Author

I think having data for unit test in the same file in the form of table makes more sense also see some k8s code that does similar things.

Similarly some more code from k8s that does similar thing.

It's very hard to map test to file. In a table where test data is also present there and if you name tests then it is very easy to goto that test and see what is missing. As opposed to opening files to find where that code is.

Also tests don't have each componenet one one line now it is contracted:

{
	Name: "One container mentioned in the spec",
	Data: []byte(`
name: test
containers:
 - image: nginx
services:
  - ports:
    - port: 8080`),
	App: &spec.App{
		Name: "test",
		PodSpecMod: spec.PodSpecMod{
			Containers: []spec.Container{{Container: api_v1.Container{Image: "nginx"}}},
		},
		Services: []spec.ServiceSpecMod{
			{Name: "test", Ports: []spec.ServicePortMod{{ServicePort: api_v1.ServicePort{Port: 8080}}}},
		},
	},
}

@surajssd
Copy link
Member Author

surajssd commented Jul 26, 2017

@surajssd simple naming conventions like test1.go, test2.go can fix that. I really don't want to be debugging huge files.

Does not make any sense! This won't make it any easier.

@surajssd
Copy link
Member Author

I think since we are dealing with configs in this project mainly, so it is expected to have tests with configs which can grow large, also please don't map size of file to complexity and ease of debugging.

Having it in separate file makes it more hard to debug, while if I have code in the same file it is easy to edit it there itself, also everytime we add new test there is less discussion on what it those variables should be named as, because all you have to do is add to list of existing test cases.

Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with @surajssd .
I think this makes much more sense, and its is much better that having a lot of small files.

@concaf
Copy link
Collaborator

concaf commented Jul 26, 2017

Got on a call with @surajssd and we agreed that let's keep one testing function in one file, instead of having multiple functions. That one function gets to keep its test table and the contents, and we have one such function per file.

TODO: add it to developer docs

@concaf concaf merged commit 6445533 into kedgeproject:master Jul 26, 2017
@kadel kadel mentioned this pull request Jul 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up the tests in encoding_test.go
4 participants