-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New task for code migration #376
base: main
Are you sure you want to change the base?
Conversation
eb9a95f
to
c95b314
Compare
// MockModelCapabilityMigrate holds a mock implementing the "Model" and the "CapabilityMigrate" interface. | ||
type MockModelCapabilityMigrate struct { | ||
*MockModel | ||
*MockCapabilityMigrate | ||
} | ||
|
||
// NewMockCapabilityMigrateNamed returns a new named mocked model. | ||
func NewMockCapabilityMigrateNamed(t *testing.T, id string) *MockModelCapabilityMigrate { | ||
return &MockModelCapabilityMigrate{ | ||
MockModel: NewMockModelNamed(t, id), | ||
MockCapabilityMigrate: NewMockCapabilityMigrate(t), | ||
} | ||
} |
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.
No need to explicitly define this. If you ever need a "combined" interface, i.e. for tests, you can just create it on the fly. I did the same thing for some provider tests.
fileName := filepath.Base(filePath) | ||
fileName = strings.TrimSuffix(fileName, language.DefaultFileExtension()) | ||
if fileName == testFileName { | ||
continue | ||
} |
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.
Is this to remove the implementation files? We don't need to remove those, right? Cause if the test file only executes code from one implementation file, we can keep the others.
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.
With both implementation files I was getting 50% coverage, after removing one of them I got 100% coverage
// Check if for each implementation file a test file exists. | ||
for implementationFileName := range implementationFileNames { | ||
if !testFileNames[implementationFileName] { | ||
return pkgerrors.Errorf("the repository %q must contain a test file for each implementation file but found none for %q", repositoryPath, implementationFileName) | ||
} | ||
} | ||
|
||
// Check if for each test file an implementation file exists. | ||
for testFileName := range testFileNames { | ||
if !implementationFileNames[testFileName] { | ||
return pkgerrors.Errorf("the repository %q must contain an implementation file for each test file but found none for %q", repositoryPath, testFileName) | ||
} | ||
} |
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.
Would maybe track them in two slices and then sort them and use slices.Equal
.
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.
It's a cleaner solution indeed. The only thing is with this one we can know if there is a missing test file for a certain implementation file and vice-versa each leads to better logging I think. Using slices.Equal
we only now that there is something missing, either a test file or a implementation file.
If you feel like that does not matter, I change it.
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.
I would still change cause it leaves less room for errors and is easier to read. Let's go clean code > usability here.
c95b314
to
4b19db1
Compare
No description provided.