This repository has been archived by the owner on Sep 6, 2024. It is now read-only.
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.
Implementing Tests #39
Implementing Tests #39
Changes from 5 commits
76b6b0b
e4f3a78
f4a2e1f
e1787d9
ea139c7
f340136
8a22cbd
4e7d073
ce0f4a7
ac8b328
8fc04b2
17bc3e3
9fa9a22
bcdaac1
7a7bd13
921705e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Good use of
before
hook here.Suggestion: Ad a check to ensure
E2E_URL
is defined and accessible: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.
The checks for toggling between languages are well-implemented.
Consider adding a short wait between clicks to ensure the page has time to update:
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.
Good setup with chromedriver. Ensure the chromedriver version is compatible with the installed Chrome version.
Idea: Add an environment variable for server_path to easily switch drivers if needed.
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.
Ensure the
E2E_URL
env var is defined with a conditional check: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.
Use
Chalk.js
which is installed already and available through some utils.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.
Maybe add a desc of what the tests are doing:
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.
Maybe add a note that they should replace the placeholder url
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.
We could have some edge cases covered?
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.
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.
.assert.urlContains(expectedSearchURL, 'Should redirect to the correct search URL')
Could use clearer language
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.
The integration tests are well-structured, and the use of helper functions enhances readability. Good job on ensuring coverage for both required and optional packages.
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.
Ensure CLI prompts correctly set these values before running the tests. Add a check that confirms the values are defined, example: