search

dominikbraun / timetrace

A simple CLI for tracking your working time.
Apache License 2.0
679 stars 75 forks source link

Create tests for `core/formatter.go` #121

Open dominikbraun opened 3 years ago

dominikbraun commented 3 years ago

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

dominikbraun commented 3 years ago

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

KonstantinGasser commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

KonstantinGasser commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.