Create tests for core/formatter.go

[dominikbraun]

There should be Table Driven Tests for core/formatter.go. They should be structured similar to the tests in core/timetrace_test.go.

[dominikbraun]

Note: Some tests for Formatter already are in core/timetrace_test.go and should be moved.

[KonstantinGasser]

I started with writing some test cases for formatter.go and moved the one test case in timetrace_test.go to formatter_test.go. Now I am wondering if we should provide a mock-up for the FileSystem in order to actually test the timetrace.go - is there a convenient way to mock this FileSystem, do you have some experience with mockups you can share with me?

[dominikbraun]

Absolutely: I planned to migrate to afero for accessing the filesystem but didn't have the time yet. First, add a field of the type afero.Fs to the fs.Fs type and extend the New function appropriately. Then replace all calls to os with calls to that afero.Fs. Finally, pass afero.NewOsFs() to fs.New in the main program and afero.NewMemMapFs() in the tests.

[KonstantinGasser]

That's a pretty cool library, thanks for the tip! I will work on that today/tomorrow.

[KonstantinGasser]

I think I will provide the implementation of the afero fs in a separate PR - might be better to separate these changes from the tests

EDIT: thought the changes would be more - change to afero fs will be included with this PR

[KonstantinGasser]

I have found a couple of places in record.go, project.go and timetrace.go where we also rely on the os or ioutil module. For the sake of segregation I will append the timetrace.Filesystem interface so all communication with the Filesystem will be done via this interface.

[KonstantinGasser]

Hi @dominikbraun, sorry for taking so long, had a lot of things going on in university.. I just came back to this issue and writing tests for the fs. One problem I see is that we are using the os, ioutil package in quite some places (project.go, record.go and timetrace.go) causing me quite some headache. IMO to test the fs it would be good to clearly separate the project, record and timetrace logic from the underlying FS.
For example instead of directly writing a new project to file in the project.SaveProject function the Fs interface should provide such logic. Maybe you have a different idea to approach this, I however, feel that testing it how it is now might introduce more bugs since afero would need to be used in many layers of the code.

What I can image is to clearly define an interface to interact with the Fs and swap the fs logic in the project,recods and timetrace file to use this interface. Do you have any thoughts about this and can assist me?

[dominikbraun]

Hi @KonstantinGasser, yes, a stronger separation between the filesystem access and the business logic probably would make sense. On this occasion, we could also separate the Filesystem interface in more concrete interfaces: ProjectFilesystem, RecordFilesystem and so on. I think that doing so will make it easier to test the individual components.

[KonstantinGasser]

Ok I will test out some implementations for that. I probably will also need to change the dependency injection then.
Since there will be a couple changes, I will create an incremental PR so it can be reviewed step by step.

[dominikbraun]

Yes, a rolling review would be nice here - and this probably will also affect some open PRs, hopefully the merge conflicts won't be too severe.