[f] Explanation system

[catriuspham]

Status

Done

Related issues

https://github.com/EastAgile/robber.py/issues/39

Description

Let's implement an explanation system, where all the failure messages will have the following format, from which the second and the third line can be omitted.

BadExpectation:
A = ...
B = ...
C = ...
Expected A to {action} B. 
{additional information}

[coveralls]

Coverage decreased (-0.9%) to 99.091% when pulling 383a181c602eb40a1b37b6bde2e01f6c55f4797b on feature/explaination-system into a7341f68a0aeaf78ef0c283a347dab245ddacd10 on master.

[coveralls]

Coverage decreased (-0.9%) to 99.091% when pulling ace7d6c3b92725b836447c87d60123a5ddff84da on feature/explaination-system into a7341f68a0aeaf78ef0c283a347dab245ddacd10 on master.

[catriuspham]

@NhanHo @hieueastagile Please take a look

[coveralls]

Coverage decreased (-0.9%) to 99.091% when pulling cb93c7ade64f3a082d73c9ef28c769222331ec50 on feature/explaination-system into a7341f68a0aeaf78ef0c283a347dab245ddacd10 on master.

[coveralls]

Coverage decreased (-0.9%) to 99.091% when pulling 45856298c3855c34452a7d31914e29254bcf1767 on feature/explaination-system into a7341f68a0aeaf78ef0c283a347dab245ddacd10 on master.

[coveralls]

Coverage decreased (-0.9%) to 99.085% when pulling f06884f98f3774dba788e979f1886955f460a222 on feature/explaination-system into a7341f68a0aeaf78ef0c283a347dab245ddacd10 on master.

[coveralls]

Coverage decreased (-0.9%) to 99.085% when pulling f06884f98f3774dba788e979f1886955f460a222 on feature/explaination-system into a7341f68a0aeaf78ef0c283a347dab245ddacd10 on master.

[hieueastagile]

Looks good to me so far, but I want to see is the explanation for comparing 2 objects (do the diff like what sure does)

[coveralls]

Coverage decreased (-0.9%) to 99.128% when pulling 3ecc9157d5e2a15555696e880ec71c5bfb73541c on feature/explaination-system into a7341f68a0aeaf78ef0c283a347dab245ddacd10 on master.

[coveralls]

Coverage decreased (-0.9%) to 99.128% when pulling 3ecc9157d5e2a15555696e880ec71c5bfb73541c on feature/explaination-system into a7341f68a0aeaf78ef0c283a347dab245ddacd10 on master.

[coveralls]

Coverage decreased (-0.9%) to 99.128% when pulling 3ecc9157d5e2a15555696e880ec71c5bfb73541c on feature/explaination-system into a7341f68a0aeaf78ef0c283a347dab245ddacd10 on master.

[catriuspham]

@oyster Could you please somehow turn off the comments from coveralls every time one pushes his code? It spams too much and I think the checks table at the end of the PR has enough information.

[oyster]

It should be configurable in Coveralls settings. Please check if you can do that, @catriuspham. Or you can ask @hieueastagile to use our open source account to do it if you don't have privilege. Thanks.

[hieueastagile]

lol, it's quite useful to me :)). Anyway, let me see if we can turn off it tomorrow.

[coveralls]

Coverage decreased (-0.7%) to 99.294% when pulling 62f77251bd29167e2c6e7e8364c7007ee412a665 on feature/explaination-system into a7341f68a0aeaf78ef0c283a347dab245ddacd10 on master.

[coveralls]

Coverage decreased (-0.5%) to 99.534% when pulling e29ad5f364c73c0ec45a63077df9adcecbc8b243 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage decreased (-0.5%) to 99.534% when pulling c8cbe3084794af0a94643a59b2161617d998df34 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage decreased (-0.5%) to 99.536% when pulling 65d9c715ec7e338c4c8410530c6ec0fc24f208ae on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage decreased (-0.5%) to 99.539% when pulling 3c4f1a02c76661cc89f7bd2f45e9eb77eafbda94 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage decreased (-0.2%) to 99.8% when pulling 900ffa205b7e2c64999f1737a8ec3edecbb9d5be on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage decreased (-0.5%) to 99.543% when pulling ce88fa19820a45439aff3d64a83246bffc3a82ec on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage decreased (-0.2%) to 99.775% when pulling 228eee53ee0aef1f8d149429c13b0b1517d33c31 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage decreased (-0.2%) to 99.778% when pulling a08886e0a8e3e5548325712da546f1ae56f81967 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage decreased (-0.2%) to 99.778% when pulling 6d914b979cee5696fc9d2cb33f0547f1593a4413 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage decreased (-0.2%) to 99.777% when pulling d816b4af1c6609629d5870c867489a82b7308bb4 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage decreased (-0.2%) to 99.777% when pulling d816b4af1c6609629d5870c867489a82b7308bb4 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage remained the same at 100.0% when pulling a2ec4c617e6c406a6fbe599604b1dd796d5bb0f5 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 813ca9b9ac0a4bda490b2a099137916621cb1871 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[catriuspham]

@hieueastagile @NhanHo @nhanbt I finished this PR, please take a look when you guys have time

[coveralls]

Coverage remained the same at 100.0% when pulling 35840081a3ad238db7ab57e5fda1b49acb4440c1 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[catriuspham]

@seri-ea would you like to take a look? :stuck_out_tongue_closed_eyes:

[seri-ea]

I will @catriuspham

[seri-ea]

@catriuspham I made some minor suggestions, but as a potential user of EA-robber.py, I find the explanation format a bit unfriendly.

I mean, why:

BadExpectation:
A = 123
B = "123"
Expected A to equal B. 

Instead of this:

Expected <int>123 to equal <string>123

And yet it doesn't support what @hieueastagile was looking for (explanation for comparing 2 objects). Sure we can and should do that in another PR as a detailed implementation of the build_diffs method, but for now I wonder if we could take a step back and re-think the message format.

[coveralls]

Coverage remained the same at 100.0% when pulling ebca4a983cf0a1c7ac8124c6d2718f1724e7e3c9 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[catriuspham]

@seri-ea the reason for the messages to have the format

BadExpectation:
A = 123
B = "123"
Expected A to equal B. 

is that sometimes the represented string of A and B can be super long, so we need to find a consistent way to make them look as clear as possible.

For example, this message

Expected <string>"Some super long texts that make your eyes bleed a lot until you faint" to equal <string>"Some super long texts that make your eyeballs bleed a lot until you faint"

look awkward.

But it's a lot better with the new format:

BadExpectation:
A = Some super long texts that make your eyes bleed a lot until you faint
B = Some super long texts that make your eyeballs bleed a lot until you faint
Expected A to equal B. 

[coveralls]

Coverage remained the same at 100.0% when pulling cf7c9c1572611898190ca4a343e79b9e4283d369 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[seri-ea]

@catriuspham Thanks for the explanation. The counter argument is that while your format makes it clear for when the string representations are long, it makes most cases harder and longer to parse (the reader has to look up A, B, C... kind of awkward), and that's probably why no popular testing library is using your failure message format yet.

Maybe only break down into lines for special cases such as the equality matcher, where you are going to have to do deep comparison and handle it exceptionally anyway.

[coveralls]

Coverage remained the same at 100.0% when pulling d9ec7e28377aef5b2f628b319419f3e3688c17cc on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage remained the same at 100.0% when pulling ad0bb05015e085ca15dadf72d9e7dc8f3c464d30 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage remained the same at 100.0% when pulling ad0bb05015e085ca15dadf72d9e7dc8f3c464d30 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage remained the same at 100.0% when pulling f500e0149abbf216fe577e7513095122909ac05a on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 4aaf098aa06ffa4d52d5aa9b81ce09ec4f7e0138 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[catriuspham]

@seri-ea Actually, the idea of this format is taken from how unittest display its failure message, it's not something that I make up myself though. I agree with you that in some cases when the represented strings of the objects are short, the message looks awkward. But on the larger scale, to me, this is the best format I can think of to cover all the cases. To detect when to use the new format, when to keep the old one would require a lot of effort though.

[hieueastagile]

@catriuspham Actually, you don't need a format which works for all cases, most of the messages are specific to its own assertion, we don't need to make it generic like you did. Anyway, I feel what you did are already good enough, not the best but usable, just let me know when you finish the diff for object/array, then we can merge it. I really want it to be merged soon and we will deploy it to pypi as robber 1.2.0. We should do another refactor later for robber 2.0, where we can think more carefully about the architecture.

[catriuspham]

@hieueastagile Please give me 1-2 day to with the none param issue. Gonna notice you as soon as I finish it.

[hieueastagile]

Take your time, no rush then @catriuspham

[catriuspham]

@hieueastagile All things are done. However, I guess we have to wait for coverage, it's been 4 hours though...

[coveralls]

Coverage remained the same at 100.0% when pulling 849000fe56a960a02e8382b9cdb7479a3abe06c4 on feature/explaination-system into b439251848fd3b5085cdac45130311bfe0facc13 on master.

[catriuspham]

@hieueastagile Everything's good. Please take a look and merge this PR whenever you want.

[hieueastagile]

@catriuspham Let me test it a bit first, I will tell anh Quang to merge when I finished. @NhanHo @nhanb @steadykhoi @hotay please help take a look on this PR also.