-
Notifications
You must be signed in to change notification settings - Fork 40
Conversation
@surajssd, thank you for the pull request! We'll ping some people to review your PR. @containscafeine, please review this. |
pkgcmd "github.com/kedgeproject/kedge/pkg/cmd" | ||
|
There was a problem hiding this comment.
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)
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. {
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 :( |
@containscafeine naming already is a problem and then naming each file will just add problems and unnecessary cognitive burden! |
@surajssd simple naming conventions like |
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.
1e181a1
to
b1f01bc
Compare
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}}}},
},
},
} |
Does not make any sense! This won't make it any easier. |
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. |
There was a problem hiding this 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.
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 |
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