Conversation
gnapse
left a comment
There was a problem hiding this comment.
This is great. Thanks!
I have two observations:
- There's still need for adding a new entry in the README documenting this new cusotm matcher.
- At the moment, CI checks are not green. More about how to solve this one below.
About CI checks not passing: you need to add assertions to the tests that make it run the negative path. That is, expectations that when you call it in a way that it should fail, it does fail.
Here are some ideas.
// usage with .not
expect(element).not.toHaveSelection(
'something that DOES NOT MATCH the current selection'
)
// same assertion without the .not should fail
expect(() => {
expect(element).toHaveSelection(
'something DOES NOT MATCH the current selection'
)
}).toThrow()
// a valid assertion with the .not in place should fail
expect(() => {
expect(element).not.toHaveSelection(
'something that DOES MATCH the current selection'
)
}).toThrow()Try to sprinkle these in some test cases.
You can then run npm run validate and it will give you a coverage report. Unless it's at 100%, CI will not pass. If you keep stuck at not being able to get it to 100% let me know and push your work anyway. I may be able to help.
|
Hello and thanks for quick feedback. I managed to increase coverage but there are still 2 lines reported. I have manually checked them with debugger and they are invoked so I could really use your help there :) |
|
Superseded by #637 Still, that one is based on the code from this PR, so I'll consider you co-author of that pull request. |
|
@all-contributors please add @pwolaq for code, test |
|
I've put up a pull request to add @pwolaq! 🎉 |
What:
Add
toHaveSelectionas requested in #289Why:
How:
Checklist:
This is still work in progress (need to update documentation and type definitions) - please let me know if you still want this feature.