-
Notifications
You must be signed in to change notification settings - Fork 449
Upgrade Jest and other test-related libraries #1212
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1212 +/- ##
=========================================
Coverage ? 75.82%
=========================================
Files ? 144
Lines ? 9321
Branches ? 2298
=========================================
Hits ? 7068
Misses ? 2012
Partials ? 241Continue to review full report at Codecov.
|
| "isThrow": false, | ||
| "value": undefined, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mock functions now track results in addition to parameters, that's why we see it in the serialized version too.
Looking at this snapshot, it looks like that addColorStop could be just a jest.fn() instead of a full spyLog in https://github.com/devtools-html/perf.html/blob/87eb4c73b3e75eea24c82e8f7950142402bd905b/src/test/fixtures/mocks/canvas-context.js#L38-L40
Indeed the data seems to be duplicated: we see both the calls to addColorStop above and the mock serialization.
What do you think ? I could do it directly in this PR or in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine however you want to do it.
| To start recording a performance profile in Firefox, first install the | ||
| <a | ||
| className={undefined} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like that when a prop has an undefined value it's not output in snapshots anymore, which seems reasonnable.
This is jestjs/jest#6162
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks cleaner.
| className="sidebar-open-close-button photon-button photon-button-ghost sidebar-open-close-button-isopen" | ||
| onClick={[Function]} | ||
| title="Open the sidebar" | ||
| title="Close the sidebar" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AH the new snapshots are actually what's expected -- this means the tests weren't working properly before :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is enzymejs/enzyme#1499 -- and this is a good thing !
Before this we had to call "update" everywhere (I just forgot to do it here). I'm gonna remove them all then ! I can't remove the call to update anywhere else because this is only for shallow and not for mount yet (maybe later ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woops.
| className="treeViewRowColumn treeViewMainColumn name" | ||
| > | ||
| <a | ||
| href="null/from-url//calltree/?file=foo%2Fbar%2Fprofile1.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again the window.location change :)
| readOnly="readOnly" | ||
| type="text" | ||
| value="about:blank" | ||
| value="http://localhost/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like window.location has a better default value now, see jestjs/jest#6792
|
All snapshot changes are actually legit. And the upgrade actually fixed a test... |
gregtatum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the upgrades!
| className="sidebar-open-close-button photon-button photon-button-ghost sidebar-open-close-button-isopen" | ||
| onClick={[Function]} | ||
| title="Open the sidebar" | ||
| title="Close the sidebar" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woops.
| To start recording a performance profile in Firefox, first install the | ||
| <a | ||
| className={undefined} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks cleaner.
| "isThrow": false, | ||
| "value": undefined, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine however you want to do it.
No description provided.