Add tests for Account, Contact, Layout and Message Stores

[frankrousseau]

I put in this PR all tests related to stores. Every file contains a comment about should be fixed in the code, once the tests will allow to perform them safely.

[m4dz]

Thanks @frankrousseau for having take the time to make this one! I find you approach to split tests into Actions and Methods a clever way to cover properly Stores behaviors.

I have still a doubt about manipulating datas, and I ask myself about the opportunity to use external fixtures files to hydrate and tests stores. What do think about it? @misstick, any advice?

[frankrousseau]

The upside of what you propose is that it makes things cleaner. But the downside is that tests data are splitted in another file and it makes tests harder to follow. My experience of testing is that it should require close to no thinking to be read. What do you think about it?

[m4dz]

Hmmm, both ways have their pros and cons. Personnally, I prefer to let things separated 'coz we can have the need of reuse same fixtures accross some tests, but your solution have the advantage of having all the stuff we need in a straigth way.

I invoke @misstick to have its opinion :smile:

[misstick]

@frankrousseau, @m4dz : I have no opinion about this; I have just seen that fixtures are not used even for server side tests: is it on purpose?

What is important is to be consistent with other cozy application: how is this handled elsewhere?

[misstick]

@frankrousseau : At first, thank you for having starting that usefull work :) I give you my first impression about this PR.

Have you seen that other client unit tests exists, but they are not used anymore: tests/client-units/account_store? They seems to me very relevant, and I think you should take a look.

To my mind, this scope of this PR is too large; there is a lot of things to do for each store: enought to make a PR for each. In example, adding tests is not only about writing new code, but also removing old un-used code that has to do the job before.

[frankrousseau]

I can't find the tests you're talking about. Are you talking about the Casper tests? These one are functional tests, they are not unit tests. They cover a broader scope but they are slow to run and unstable. They are different kind of tests, we should not compare them to unit tests.

About removing / cleaning the code, we need tests first. Once the unit tests will be written, it will be much easier to remove or rewrite store code because you will immediately notice if you introduce regressions or if you break something.

[misstick]

@frankrousseau : about previous tests, the absolute path is: /client/tests/client-units/account_store and its not functional tests as you can see.

[misstick]

@frankrousseau : yes, I understand that. But I'm not talking about removing/cleansing dead code into application but into tests. Look at my previous comment above.

[frankrousseau]

I can't find them: https://github.com/cozy/cozy-emails/tree/master/client/tests

[misstick]

@frankrousseau: My bad, the path was falsy :/ Try this: https://github.com/cozy/cozy-emails/tree/development/tests/client-units

[frankrousseau]

Again I think they are a different kind of tests. These tests are functionnal too (based on a scenario). What I did are very local tests which can be called unit tests. They are complementary' we should not oppose them.

[misstick]

@frankrousseau: Ok. But at first we wanted to clean files, and these functionnal tests are into tests/client-units directory; this doesnt make sense, dont you think so? But we can move them into an other PR.

[frankrousseau]

Yes, the folder name is misleading. I will rename it in another PR dedicated to improvements.

[misstick]

@frankrousseau, @m4dz: about fixtures, here are fixtures /tests/fixtures that dont seem to be used; I can see into server side test includes cozy-fixtures but its not the same thing, isnt it? Should we remove these files to remove ambiguity if we don't use fixtures at all?

[frankrousseau]

Fixtures for server are different. They aimed to be stored in database and used as is. Here we talk about local memory fixtures. I'm not sure it's a good idea to try to mix them, because use cases are different and they are designed to work with the cozy-fixtures tool. I will take a look at it.

[misstick]

@frankrousseau: (global comment) I noticed that this PR is adding unit test for some Stores but not all of them; why? Can you specify it into PR description please?

I usually add checklist into my PR description because I find them more clear than big sentences as you please :)

[m4dz]

I'm not sure it's a good idea to try to mix them, because use cases are different

I totally agree, fixtures sets are dedicated to their parts (server vs client) and we shouldn't mix them.

[misstick]

@frankrousseau : I'm waiting about your fixes so that I can merge this PR :)

[frankrousseau]

As @misstick and @m4dz requested, I did some improvements:

• Fix mockery utils usage;
• Put fixtures on top of the test file for each test file;
• Remove useless skipped tests of private methods;
• Better formation of problems found in store code while writing tests;
• Fix addAccount typo in the function name.

I you agree with theses fixes, we should merge this PR and tackle other problems, like the unit-test folder renaming, in another PR.

[misstick]

@frankrousseau: it's ashame that you didn't commit fix and enhancement with specific commit prefixed with [review].

I would have been more easy to read your changes :(

Anyway, it's OK for me 👍 Great job! It is the 1rst step of a very usefull work.

[frankrousseau]

@misstick I didn't see any rules related to prefixing or commiting. So, I didn't guess that [review] should be used.

Sorry, for the melted commits but my context was not right to do it (I had separated branch for tests configuration changes and tests related to accounts). Another thing is that writing tests on code I didn't write is super boring, so making it perfect is even more painful. Now you know that, I hope you will understand my tradeoffs about quality.