I think this may be omitted. There's also PrivateTag, but testing events for all synchronised settings looks superfluous, having tests for all settings set locally + one for synchronisation, say, of Displayname, is good enough, all the logic for account events is implemented in the only place anyway

is private tag synced? I thought it was only local

Private tag is not synchronized, at least currently.

the idea was to test everything that changes in jsonrpc Account type:

(except for address and color, address is not shown anymore in account list and I doubt that color changes that often (maybe with aeap when address changes? but we can add a test for that then, when this is more common))

Ok, private tag isn't synced, but this isn't important. The idea is to avoid combinatorial explosion of testcases:

• We already have tests on synchronisation of Displayname and Selfavatar
• You add also tests that account events for them are emitted as expected
• Then it's sufficient to have only one test that account events are emitted on the second device for a synchronised setting, no need to test this for all settings, moreover all the core logic is implemented in the only place.

EDIT: So, if a test doesn't cover anything in addition to the existing tests (at least theoretically), it better should be removed, otherwise it's just an extra code to maintain.

I don't agree that tests should rely on implementation details or on other tests of different files. IMO tests should test all cases. how it's implemented shouldn't matter to the tests.

We could add the event check to the old test and remove the new test, but then we would have less separation of concern - so I think it's ok to have duplicated test code.

I also don't like the idea of "only white-box testing", i just can't imagine how this code block can test anything in addition to other tests (i.e. how the core code should be changed to create a new uncovered path). And if we don't want to rely on what other tests do, we always need to go through all cases combinatorially which is just impossible.

We could add the event check to the old test and remove the new test

I don't suggest to remove the whole test, only the block for Selfavatar. And yes, we can add event checks to some existing tests (even just in case), that will still be less code.

This is not critical for merging this PR of course, i just suggest to find some practical compromise for the amount of test code.

we could remove the checks if it worked and just test if the action triggered the event. But I still think the more tests the better.
Also this is not very hard to maintain code in my view, the code is straight forward and easy to understand in this case.

i am lost a bit in the exact discussion, but, in general, i agree with @Simon-Laux - the more tests the better.

when i write tests for sth, i do not care if the things i assert() are already checked elsewhere, or if some assert()s are not strictly related. i even think, there is worth in different tests about the same thing, esp when written by different ppl. some test may change - or do not cover the exact same cornercase, this is not always obvious, and often also not worth to think about.

i would also not like to have too much conventions/compliance how to do tests etc. -- that would only lead to less tests written. review should focus on the implementation mostly - and complain about missing tests

Could also call .clear_events() in between to make sure we don't accidentally succeed because previous action emitted event twice.