test: add support for automated tests for frontend with Jest and tests for HomeLoggedOut and NetworkHeader components #136
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Looks good to me |
|
The only problem that I see is the mixture of the ES6 and CommonJS modules. Is something broken with ES6? It is better to migrate fully to the ES6. So the |
davidlag0
commented
Jan 4, 2023
|
There you go, should be better now and all tests still pass. Would you mind keeping issue #135 opened until all tests have been built for the frontend? |
davidlag0
changed the title
Yes, certainly Are you going to open the new PR with other frontend tests so I should merge it now, or we could just change the current PR title? |
|
Yes! It will take me time to create all tests. It went somewhat quickly for the 2 components included in this PR because they were somewhat small but with more complex ones, it could take me weeks to cover them well. I already updated the PR title with the additional tests I added this morning and if you agree, this PR could be merged and I would open additional PR's as I move along with more tests until issue #136 is fully resolved. |
dec0dOS
reviewed
Jan 4, 2023
| @@ -0,0 +1,59 @@ | |||
| import { render, screen } from "@testing-library/react"; | |||
| import HomeLoggedOut from "../../src/components/HomeLoggedOut"; | |||
There was a problem hiding this comment.
I think there is a better way to import the components
|
I think that it's better to have an import path like |
|
To be honest, I thought about it, I hesitated and when I saw the other imports did not have anything before them and were names like |
|
Is there any additional change you would like to see for this PR? |
dec0dOS
reviewed
Jan 10, 2023
frontend/package.json
Outdated
| "source-map-explorer": "^2.5.2" | ||
| }, | ||
| "scripts": { | ||
| "start": "BROWSER=none react-scripts start", | ||
| "build": "react-scripts build", | ||
| "analyze": "source-map-explorer 'build/static/js/*.js'" | ||
| "analyze": "source-map-explorer 'build/static/js/*.js'", | ||
| "test:unit": "npx jest --coverage --testPathPattern='unit'" |
There was a problem hiding this comment.
I think that it would be better to install jest in devDependencies and use it directly. ZeroUI uses the yarn package manager, so it's a bit confusing to see the npx.
Thanks!
There was a problem hiding this comment.
Ok, sorry about this! I realized another package was missing too after adding Jest so I added that other package as well (jest-environment-jsdom). Let me know if you see anything else that needs to be changed!
This error message appeared before the installation of this package resolved the issue: SecurityError: localStorage is not available for opaque origins
|
Seems to be something is wrong now. |
I came across this, but it is technically unrelated to the PR. I can make the changes anyways if you want to see the errors gone. I did a successful test change with |
|
Ok, that's fine, I could merge it now. |
|
Thank you very much! I'll proceed to add more tests! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request type
Please check the type of change your PR introduces:
Support for automated tests with Jest for the frontend only.
What is the current behavior?
There are currently no automated tests with the codebase. This pull request is meant to add such support for the frontend only for now using Jest and other related tools and to provide the tests for the first component: HomeLoggedOut.
Issue Number: #135
What is the new behavior?
Automated tests will now be supported for the frontend. More tests are required to fully cover the codebase but this is a starting point to eventually enable GitHub's dependabot, automated tests with GitHub Actions, etc.
Does this introduce a breaking change?
The changes are mainly related to development-related functionalities.
package.jsonis updated to add the required additional dev dependencies and so isyarn.lockto reflect these changes.Other information
Don't hesitate to ask me any question you might have about the change.