Goodtime icon indicating copy to clipboard operation
Goodtime copied to clipboard

Add tests

Open fdw opened this issue 7 years ago • 6 comments

Unfortunately, Goodtime has no tests. This makes code changes much more challenging than it needs to be (as I found out the hard way ;)). It would be very useful to have automated unit and functional tests. A CI-infrastructure with Travis is already in place, so all we need are tests.

fdw avatar Jan 04 '17 16:01 fdw

As you can see, I already gave unit tests a whirl with this branch. However, most of the other code has a very high complexity that makes it nearly impossible to write good unit tests. Do you think the productive code should be simplified so that we can test it, @adrcotfas ?

fdw avatar Jan 04 '17 16:01 fdw

Of course, simplifying the code is always welcome.

adrcotfas avatar Jan 05 '17 08:01 adrcotfas

Hi, it's been quite some time since I opened this, so I wanted to give you a heads-up that I'm still working on this, and a quick update: All of the logic regarding the interface is in the TimerActivity, so I think testing it is very important. However it's also not easy, because tests need to test both the behavior ("is the timer running?") and the UI ("Is the button now visible?"). To get a better grip on this, I tried to split the class according to the standard design pattern for Android, Model-View-Presenter, and have both an TimerActivity and a TimerPresenter. Also, since the activity is both for the sidebar and the timer UI itself, I wanted to create a TimerFragment containing only the timer UI. In theory, this means that each class has a single, well-defined responsibility, and testing should be much easier (the TimerPresenter needs no Android-specific framework, for example, and the TimerFragment should have no logic tests). To ease the pain of dependencies, I wanted to use Dagger 2 which has no runtime cost. All of this seems to be best practice for Android apps, but I'm not sure - what do you think of this?

I tried to build a proof of concept for these ideas with some dirty refactorings, but it's not that easy - mostly because I don't have any experience with Dagger. While most of the separate classes look okay and have sane interfaces, there is still a circular dependency that I can't seem to resolve. I will keep trying to work this out, but I wanted your opinion on the whole design before I put more work into it. If you're interested in the code, I can push what I have.

Additionally, I've started this proof of concept more than a month ago, so I missed all your changes since then :( But I hope that a working prototype can be extended and polished quite easily.

fdw avatar Feb 16 '17 18:02 fdw

Hi, You're plan sounds good and I look forward to see what you manage to refactor. I don't have any experience with Dagger either but if you need any help, please let me know! You could split the work in small tasks in the issue page if you want. In the past month the changes weren't too numerous I would say and I hope that they will not entangle you :)

adrcotfas avatar Feb 16 '17 20:02 adrcotfas

Thanks for your kind words and support! :)

I pushed the non-working proof of concept to a separate branch so you can have a look at it. Please remember that it is not good code, but so much copy-and-pasting and just generally prodding to test if it might work!

I think the best way forward would be to start the redesign all over again, but with much smaller and better defined steps - trying to do everything at once was a bad idea. As you can see here, I rebased onto your latest changes and then moved some classes around to have a clearer package structure. From this I'm then going to implement a presenter for TimerActivity. Only when this is working should we start to separate the TimerFragment. How does this sound to you?

fdw avatar Feb 19 '17 12:02 fdw

Sounds good! I left some comments to your changes. BTW, I see I forgot to merge some of my recent changes and hopefully they will not affect your new branch too much. I left an explanation of the change here: 0791fc17a96f2427a1e5c90c50d48c905c4368a6. I will merge it now to have the current state of the app on master.

adrcotfas avatar Feb 19 '17 14:02 adrcotfas