-
Notifications
You must be signed in to change notification settings - Fork 10
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
added tests for button #85
base: dev/gdsc-open-source-2022
Are you sure you want to change the base?
Conversation
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.
LGTM, I prefer a singular test directory instead of per component but I haven't worked a lot with client testing. @hiimchrislim thoughts?
Oh wait i just noticed this, the base branch is incorrect please change it to dev/open-source one pls |
import userEvent from "@testing-library/user-event"; | ||
|
||
test("displays value as button label", () => { | ||
// Arrange |
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.
Lets remove the // Act
, // Arrange
and // Assert
comments.
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.
Additionally, we can remove a lot of the whitespaces/blank lines.
@@ -79,6 +79,7 @@ | |||
"eslint-plugin-promise": "^5.1.0", | |||
"eslint-plugin-react": "^7.26.1", | |||
"husky": "^7.0.2", | |||
"jest": "^26.6.0", |
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.
Since this is the first set of tests that we have, can you also add the workflow for running the tests as well?
Yea, per component / file should be fine to have for client testing. I prefer it per component since it'll be a unit 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.
Overall, it looks pretty good. I just left a few comments to take a look at.
Also take a look at Shubh's comments.
|
||
test("displays value as button label", () => { | ||
// Arrange | ||
const value = "Some Label"; |
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.
Lets make the props for the button globally accessible (throughout the file) so we don't have to keep defining it over and over again. (i.e. callbackMock
, value
and className
should ideally only be defined once and then reused wherever it needs to be reused)
import userEvent from "@testing-library/user-event"; | ||
|
||
test("displays value as button label", () => { | ||
// Arrange |
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.
Additionally, we can remove a lot of the whitespaces/blank lines.
I wrote tests for the button component that tests all the props and functionality required.