Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add toHaveSelection #412

Closed
wants to merge 4 commits into from
Closed

Conversation

pwolaq
Copy link
Contributor

@pwolaq pwolaq commented Oct 15, 2021

What:

Add toHaveSelection as requested in #289

Why:

How:

Checklist:

  • Documentation
  • Tests
  • Updated Type Definitions
  • Ready to be merged

This is still work in progress (need to update documentation and type definitions) - please let me know if you still want this feature.

@gnapse gnapse self-requested a review October 16, 2021 11:52
Copy link
Member

@gnapse gnapse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. Thanks!

I have two observations:

  1. There's still need for adding a new entry in the README documenting this new cusotm matcher.
  2. 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.

src/utils.js Outdated Show resolved Hide resolved
@pwolaq
Copy link
Contributor Author

pwolaq commented Oct 17, 2021

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 :)

@silviuaavram silviuaavram mentioned this pull request Oct 8, 2024
4 tasks
@gnapse
Copy link
Member

gnapse commented Oct 16, 2024

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.

@gnapse
Copy link
Member

gnapse commented Oct 16, 2024

@all-contributors please add @pwolaq for code, test

Copy link
Contributor

@gnapse

I've put up a pull request to add @pwolaq! 🎉

@gnapse gnapse closed this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants