chaoss / grimoirelab-elk

GNU General Public License v3.0
59 stars 121 forks source link

[tests/data] Improve telegram coverage #971

Closed VSevagen closed 3 years ago

VSevagen commented 3 years ago

Coverage was missing for line 133-134. This commit fixes that.

Signed-off-by: sevagenv sevagenv@gmail.com

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 725266482


Files with Coverage Reduction New Missed Lines %
/home/runner/work/grimoirelab-elk/grimoirelab-elk/grimoire_elk/enriched/github.py 2 75.0%
<!-- Total: 2 -->
Totals Coverage Status
Change from base Build 718888590: 0.0%
Covered Lines: 8612
Relevant Lines: 10469

💛 - Coveralls
VSevagen commented 3 years ago

How does it exactly improves the coverage? What cases are checked here that weren't before?

the elif condition on line 133 of telegram.py was never covered as the test case for it was not present in the json file. I just added an additional object without the text field but with the sticker field to trigger the sticker condition on line 133.

sduenas commented 3 years ago

How does it exactly improves the coverage? What cases are checked here that weren't before?

the elif condition on line 133 of telegram.py was never covered as the test case for it was not present in the json file. I just added an additional object without the text field but with the sticker field to trigger the sticker condition on line 133.

In that case, I think it's better to create a pair of tests where it's checked whether the reply_to_message contains the sticker or the text. Covering the case doesn't mean the case is working properly.

VSevagen commented 3 years ago

Sorry for the delay @sduenas I made a pair of tests to check for message and sticker. text is not really available as it get maps to message.

VSevagen commented 3 years ago

@sduenas I updated the PR according to what you said. Not really sure if that's what you meant but I think I got it this time

sduenas commented 3 years ago

@sduenas I updated the PR according to what you said. Not really sure if that's what you meant but I think I got it this time

Kinda of. What I meant is. We know what item have the sticker data on it, so check that for that specific item, the exact value (the exact string) is there. It would be something like:

item = self.items[x] <- change x by the index number of the item with the sticker
[...]
self.assertEqual(message['reply_to_message']['sticker'], "sticker value") <- change 'sticker value' for the real value
VSevagen commented 3 years ago

@sduenas but we're checking for sticker in reply_to_message. Right now, i think there is no item that has it. There are items that has sticker but not reply_to_message and vice versa. I was checking telegram.json. Should I create an item ?

sduenas commented 3 years ago

@sduenas but we're checking for sticker in reply_to_message. Right now, i think there is no item that has it. There are items that has sticker but not reply_to_message and vice versa. I was checking telegram.json. Should I create an item ?

Yes, that's it. If there are no item with a value, let's create it because otherwise we are testing nothing.

VSevagen commented 3 years ago

@sduenas I have a doubt regarding the sticker value. I didn't know how the sticker format would be like so i checked one existing format in the same json file. I'm not seeing any value for sticker. Its mostly a collection of other fields and values. So to see what was being asserted, I checked the value for message[reply_to_message][sticker]. That turned out to be the collection of fields in sticker. Am I confusing something ?

I also updated the PR with the new item.

sduenas commented 3 years ago

@sduenas I have a doubt regarding the sticker value. I didn't know how the sticker format would be like so i checked one existing format in the same json file. I'm not seeing any value for sticker. Its mostly a collection of other fields and values. So to see what was being asserted, I checked the value for message[reply_to_message][sticker]. That turned out to be the collection of fields in sticker. Am I confusing something ?

I also updated the PR with the new item.

I think we can leave it that way. The PR looks good to me.

sduenas commented 3 years ago

Merged. Thanks for your contribution.