Skip to content
New issue

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

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

Already on GitHub? # to your account

[expect] optimize compare nodes #8368

Merged
merged 3 commits into from
Apr 27, 2019

Conversation

Connormiha
Copy link
Contributor

Summary

Removed needless checking for isEqualNode because it checked before.
Used lazy checking for second object.

Test plan

@scotthovestadt
Copy link
Contributor

Thanks for this! Obviously the check is completely unnecessary in some cases. Questions:

  1. Where did typeof a.isEqualNode === 'function' go? It looks necessary at a quick glance.
  2. Any idea how expensive isDomNode is in terms of ops/sec? Just curious.

@Connormiha
Copy link
Contributor Author

Where did typeof a.isEqualNode === 'function' go? It looks necessary at a quick glance.

This has already been checked inside isDomNode

Any idea how expensive isDomNode is in terms of ops/sec? Just curious.

Very cheap, but expect can be inside for loop is some test cases.

@scotthovestadt

@SimenB SimenB requested a review from pedrottimark April 24, 2019 06:28
@pedrottimark
Copy link
Contributor

Thank you for improving the calling code after my too-hurried change to isDomNode in #8064

@pedrottimark
Copy link
Contributor

@Connormiha Forgot to notice no line under Chore & Maintenance in CHANGELOG.md

Can you please merge changes from jest/master to avoid conflict, and then add the line, so I can merge your contribution.

@Connormiha Connormiha force-pushed the optimize-check-dom-node branch from 5c5de91 to 9533e4c Compare April 26, 2019 22:58
@Connormiha
Copy link
Contributor Author

I did rebase from master @pedrottimark

@codecov-io
Copy link

Codecov Report

Merging #8368 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8368      +/-   ##
==========================================
- Coverage   62.35%   62.34%   -0.01%     
==========================================
  Files         266      266              
  Lines       10740    10738       -2     
  Branches     2611     2611              
==========================================
- Hits         6697     6695       -2     
  Misses       3460     3460              
  Partials      583      583
Impacted Files Coverage Δ
packages/expect/src/jasmineUtils.ts 91.07% <0%> (-0.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b6464d...9533e4c. Read the comment docs.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants