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 tests to verify error messages thrown by commands #25

Merged
merged 6 commits into from
Dec 1, 2018

Conversation

misoguy
Copy link
Contributor

@misoguy misoguy commented Nov 21, 2018

This PR adds tests to verify this issue I added in https://github.com/kentcdodds/dom-testing-library

This PR is intended to fail.
When the issue at dom-testing-library resolves and the dependency is updated with a new release, this test should pass.

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Sweet! Thank you!

Cypress uses chai for assertion causing the lint to fail
@misoguy
Copy link
Contributor Author

misoguy commented Nov 21, 2018

Also, the test getAllByText fails on cypress 3.1.2 but passes on 3.0.1 which causes the test to fail on CI.

@misoguy
Copy link
Contributor Author

misoguy commented Nov 21, 2018

I just confirmed that it works on 3.1.1. It seems like something broke on 3.1.2.
I'll add an issue on cypress regarding this break.

@kentcdodds
Copy link
Member

Let's keep this open until dom-testing-library had been updated and published here

@kentcdodds kentcdodds reopened this Nov 21, 2018
@misoguy
Copy link
Contributor Author

misoguy commented Nov 21, 2018

Yeah, that's what I was thinking. When dom-testing-library is published I'll update the dependency in this PR and add contributor as well :)

@kentcdodds
Copy link
Member

Should be ready

@misoguy
Copy link
Contributor Author

misoguy commented Nov 21, 2018

@kentcdodds Dependency is updated 👍 I think the CI will still fail on one test due to the issue with cypress

@kuceb
Copy link

kuceb commented Nov 21, 2018

@kentcdodds @misoguy One of your dom utilities is yielding an array of native dom elements, and cypress commands expect jquery element arrays. Cypress previously wrapped the array in jquery but this was undefined behavior, thus it was not captured in a spec and changed unknowingly in 3.1.2.

We've decided to add that functionality to the spec and will have this fix out in a patch release. so your tests will pass after the next patch release

see cypress-io/cypress#2820 (comment)

@misoguy
Copy link
Contributor Author

misoguy commented Nov 21, 2018

@bkucera Thank you!

@misoguy
Copy link
Contributor Author

misoguy commented Nov 30, 2018

@kentcdodds Could we have this merged and released? The issue on cypress seems to be taking a bit more time than anticipated and my tests are spitting too much dom nodes on failing tests. 😞 We could fix the cypress devDependencies on this repo to 3.1.1 which would fix the test failing in CI temporarily.

@kentcdodds
Copy link
Member

It can't be released until CI is passing (releases are automated).

@misoguy
Copy link
Contributor Author

misoguy commented Dec 1, 2018

CI will pass if we change the version of cypress in the devDependencies on this repo to a fixed version 3.1.1. But I'm not sure if you are ok with that change or you would like to wait for cypress to fix the issue.

@kentcdodds
Copy link
Member

Let's go ahead and lock the version for now

@misoguy
Copy link
Contributor Author

misoguy commented Dec 1, 2018

I locked the cypress version and had to update a snapshot testing due to dom-testing-library version update.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@misoguy
Copy link
Contributor Author

misoguy commented Dec 1, 2018

My pleasure 😄

@kentcdodds kentcdodds merged commit 676426c into testing-library:master Dec 1, 2018
@kentcdodds
Copy link
Member

🎉 This PR is included in version 2.3.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants