jsumners / alfred-emoji

Alfred workflow for searching and copying emoji
741 stars 32 forks source link

test(search): improve test coverage #48

Closed oscard0m closed 3 years ago

oscard0m commented 3 years ago

πŸ“ Summary

β›± Motivation and Context

To reach 100% test coverage

jsumners commented 3 years ago

Is this still in draft or are you okay with not covering line 65?

oscard0m commented 3 years ago

Is this still in draft or are you okay with not covering line 65?

I was expecting to land this since I did not find an easy way to cover that line:

https://github.com/jsumners/alfred-emoji/blob/d054a711884caa1cda45a6fd105ffc0e12cd60f6/src/search.js#L61-L69

It depends on emojilib and I did not find a way to inject or mock with tap so I can force that path of execution. Maybe you have more experience with it and you can provide some guidance here? πŸ‘ΌπŸ½

jsumners commented 3 years ago

I would use https://www.npmjs.com/package/mock-require

oscard0m commented 3 years ago

I would use npmjs.com/package/mock-require

@jsumners

Working with your package proposal and so far so good. With current mocked emojilib I'm facing problems giving test coverage for the following lines:

Coverage for Line 26

https://github.com/jsumners/alfred-emoji/blob/d054a711884caa1cda45a6fd105ffc0e12cd60f6/src/search.js#L23-L27

Checking the code and the possible values in emojisMock.json, char property will always be a string, and, consequently, the .match expression will always be truthy, right? Checking the commit adding this lines did not give to me any clue on what is this logic for. Any clue?

(strange because with previous version of my tests I made it to cover this line but I don't know exactly how πŸ™„)

Coverage for Line 65

https://github.com/jsumners/alfred-emoji/blob/d054a711884caa1cda45a6fd105ffc0e12cd60f6/src/search.js#L61-L80

According to the invocation of matches() and after it, alfredItems https://github.com/jsumners/alfred-emoji/blob/d054a711884caa1cda45a6fd105ffc0e12cd60f6/src/search.js#L94

I think there is no path of execution where line 65 can be falsy so that it stops execution there. Could that be possible? If so, do you have any example where that could happen, so I can reproduce it in the mock?

Thanks for your support and help here :)

jsumners commented 3 years ago

Checking the code and the possible values in emojisMock.json, char property will always be a string, and, consequently, the .match expression will always be truthy, right? Checking the commit adding this lines did not give to me any clue on what is this logic for. Any clue?

Maybe @Glennmen can help answer this question. It was added by https://github.com/jsumners/alfred-emoji/commit/2973921b00747ef706d16c838becd47811bb3db0

I think there is no path of execution where line 65 can be falsy so that it stops execution there.

I believe the thinking is that if one searches for "baz" then it will short circuit and not attempt to add any search results for Alfred to display.

Glennmen commented 3 years ago

The logic I got for the skintone modifier came from: https://github.com/muan/mojibar/blob/3a1dac0998202e8425a63acb37da82aa7d6385f9/app/search.js#L225

Not sure about line 65 through, can't remember if I actually checked if that is ever triggered. Anyways this test should catch if a none valid search is passed: https://github.com/jsumners/alfred-emoji/blob/d054a711884caa1cda45a6fd105ffc0e12cd60f6/test/search.test.js#L24-L28

It has been a while sorry πŸ˜… if there is something else I can help with please let me know.

oscard0m commented 3 years ago

Finally found the use case for L26 πŸ₯³ Here is the key commit to find it: https://github.com/muan/mojibar/commit/b4c1f47f08ad45b1335e9f5d77bc18fa452eb8e9 And here is the info I found about ZWJ emojis: https://emojipedia.org/emoji-zwj-sequence/

oscard0m commented 3 years ago

To make test to cover L65 and do not fail for the rest I had to add an extreme use case:

To avoid exploding for the rest of tests, I had to add an extra defensive programming statement:

emojilib.lib[name] &&
    emojilib.lib[name].keywords.some((keyword) => keyword.includes(term))