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

Fix weird output :) #295

Merged
merged 7 commits into from Oct 31, 2019
Merged

Fix weird output :) #295

merged 7 commits into from Oct 31, 2019

Conversation

papandreou
Copy link
Member

Fixes #294

@papandreou papandreou self-assigned this Oct 26, 2019
@papandreou
Copy link
Member Author

Hmm, this is causing one of the examples in the documentation to fail differently:

diff --git a/documentation/assertions/DOMNodeList/to-contain.md b/documentation/assertions/DOMNodeList/to-contain.md
index 3351913..f471c71 100644
--- a/documentation/assertions/DOMNodeList/to-contain.md
+++ b/documentation/assertions/DOMNodeList/to-contain.md
@@ -25,49 +25,54 @@ expect(element, 'to contain', { name: 'li', textContent: /One|Two|Tree/ });
 In case of a failing expectation you get the following output:
 
 ```diff
 expect(
   element,
   'queried for',
   'li',
   'to contain',
   '<li class="count">Three</li>'
 );
 ```
 
 ```output
 expected
 <section>
   <h1>Numbers</h1>
   <hr>
   <ol data-test-id="numbers">
     <li class="number">...</li>
     <li class="number">...</li>
     <li class="number">...</li>
   </ol>
 </section>
 queried for li to contain '<li class="count">Three</li>'
   expected
   NodeList[
     <li class="number">One</li>,
     <li class="number">Two</li>,
     <li class="number">Three</li>
   ]
   to contain <li class="count">Three</li>
 
   <li
     class="number" // expected [ 'number' ] to contain 'count'
-  >Three</li>
+  >
+    One // should equal Three
+        //
+        // -One
+        // +Three
+  </li>
 ```

Seems like it's now considering the match against the <li ...>One</li> equally bad as the one against <li ...>Three</li>, even though it has one more difference. I'll 👀

@papandreou
Copy link
Member Author

Figured out that scoreElementAgainstSpec expected that the spec does not contain DOM nodes and added a fix for that. I wonder if it would be better to add a mode to convertDOMNodeToSatisfySpec where it does it the old way. Depends on how "hot" this code is, @sunesimonsen?

@coveralls
Copy link

coveralls commented Oct 26, 2019

Pull Request Test Coverage Report for Build 1233

  • 14 of 14 (100.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.09%) to 93.515%

Files with Coverage Reduction New Missed Lines %
src/index.js 1 94.48%
Totals Coverage Status
Change from base Build 1221: -0.09%
Covered Lines: 683
Relevant Lines: 713

💛 - Coveralls

test/index.spec.js Outdated Show resolved Hide resolved
@alexjeffburke
Copy link
Member

@papandreou minor comment about the test but this looks like a nice fix.

@papandreou
Copy link
Member Author

Yeah, I agree, I had forgotten that we had those helpers 😅

Fixed in 1aeca8b

Copy link
Member

@sunesimonsen sunesimonsen 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 beautiful 💯

@sunesimonsen
Copy link
Member

Figured out that scoreElementAgainstSpec expected that the spec does not contain DOM nodes and added a fix for that. I wonder if it would be better to add a mode to convertDOMNodeToSatisfySpec where it does it the old way. Depends on how "hot" this code is, @sunesimonsen?

@papandreou I think the to contain case is already very hot, so I think it makes sense to special-case it.

Just for the duration of each outer findMatchesWithGoodScore call

#295 (comment)
@papandreou
Copy link
Member Author

papandreou commented Oct 26, 2019

@i think the to contain case is already very hot, so I think it makes sense to special-case it.

Hmm, unfortunately that re-uglifies some of the diffs for to contain, so I think we have to figure something else out. I applied memoization in 3e757e1, that should get us back to the previous performance.

@papandreou
Copy link
Member Author

Map should be available in IE11 🤞

@sunesimonsen
Copy link
Member

@papandreou the problem with to contain is that we will try to match it against all nodes in the tree, that is why I have made special code that will exit early, when I find something that looks good enough, I use the normal to satisfy.

@sunesimonsen
Copy link
Member

But the current PR doesn't look like it will make that situation a lot worse.

@papandreou
Copy link
Member Author

papandreou commented Oct 26, 2019

But the current PR doesn't look like it will make that situation a lot worse.

You mean with or without the memoization? 🤔

@papandreou
Copy link
Member Author

@sunesimonsen, I'll merge it as-is, we can just revert the memoization if that's what you meant. It's not exactly pretty :)

@papandreou papandreou merged commit 6341981 into master Oct 31, 2019
@papandreou papandreou deleted the fix/294 branch October 31, 2019 22:44
@papandreou
Copy link
Member Author

Released in 4.14.2.

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.

Weird output when satisfying an string against an element
4 participants