-
Notifications
You must be signed in to change notification settings - Fork 410
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
Upgrade Jest and other test-related libraries #1212
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1212 +/- ##
=========================================
Coverage ? 75.82%
=========================================
Files ? 144
Lines ? 9321
Branches ? 2298
=========================================
Hits ? 7068
Misses ? 2012
Partials ? 241 Continue 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.
@@ -334,7 +334,6 @@ exports[`app/Home renders the install screen for the legacy add-on 1`] = ` | |||
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.
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.
@@ -222,7 +222,7 @@ exports[`calltree/ZipFileTree renders a zip file tree 1`] = ` | |||
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 :)
@@ -145,7 +145,7 @@ exports[`app/MenuButtons renders the MenuButtons buttons 1`] = ` | |||
className="menuButtonsPermalinkTextField" | |||
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... |
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!
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.
@@ -334,7 +334,6 @@ exports[`app/Home renders the install screen for the legacy add-on 1`] = ` | |||
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.