-
Notifications
You must be signed in to change notification settings - Fork 68
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
chore: Test frontend/pages/sites/SiteListItem.jsx (#4965) and fronten… #4696
chore: Test frontend/pages/sites/SiteListItem.jsx (#4965) and fronten… #4696
Conversation
|
||
import SiteListItem from './SiteListItem'; | ||
// import GitHubLink from '@shared/GitHubLink'; | ||
// import PublishedState from './PublishedState'; |
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.
Should I be stubbing these instead?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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 had to do this slightly differently with expect.objectContaining() and expect.anything()
LMK what you think
@@ -86,11 +63,6 @@ SiteListItem.propTypes = { | |||
viewLink: PropTypes.string, |
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 not sure about viewLink
-- i wonder if it was what the link used to link to before we decided to make it the build history?
🤖 This is an automated code coverage reportTotal coverage (lines): 17.94% |
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.
Great work and solid additions to our frontend tests! I just put in a few comments to simplify/breakout some of the test supports.
|
||
import SiteListItem from './SiteListItem'; | ||
// import GitHubLink from '@shared/GitHubLink'; | ||
// import PublishedState from './PublishedState'; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
f0d20f1
to
962f2bd
Compare
0b6cdb3
to
a55c7ee
Compare
… frontend/pages/sites/PublishedState.jsx
a55c7ee
to
0a53abf
Compare
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 looks good to go to me. Nice refactoring and readable tests.
Changes proposed in this pull request:
<Link>
directlysecurity considerations
N/A - just a few tests