-
Notifications
You must be signed in to change notification settings - Fork 17
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
refactor: extract login from Cypress tests #981
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for findadoc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
263c85e
to
fe4eeaf
Compare
032c510
to
6b82aff
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.
@gminetoma great work. Just a few initial questions
.yarnrc.yml
Outdated
@@ -5,3 +5,6 @@ enableGlobalCache: false | |||
nodeLinker: node-modules | |||
|
|||
yarnPath: .yarn/releases/yarn-4.4.0.cjs | |||
|
|||
injectEnvironmentFiles: | |||
- .env.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.
Should this be changed to github secrets for the CI/CD?
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 way it is now (with the env file and the Cypress action config) is working because yarn is loading the env variables. This should be changed if don't have the env file anymore.
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 feel like there's a better option here like importing through the yarn command in package.json or through the yaml file 🤔
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.
Those are fine to add 👍
we're actively using both of them in our projects already
480d9c7
to
f50356c
Compare
@@ -94,32 +94,12 @@ export default withNuxt( | |||
'vue/html-indent': ['error', 4] | |||
} | |||
}, | |||
// Linting for tests (cypress + pinia) | |||
// Linting for Cypress (https://www.npmjs.com/package/eslint-plugin-cypress) |
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 want to keep support for our pinia tests so we should split this config section into two then. One for cypress and one for pinia files.
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.
cypress: pluginCypress | ||
}, | ||
files: ['test/cypress/**/*'], | ||
...pluginCypress.configs.recommended, |
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.
Did you check that we actually want all the rules from here? I haven't checked cypress recommended, but it's pretty common they can be more annoying than helpful.
I'm ok with it if we check they make sense first 🙏
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.
Please confirm on this one
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 are not using any rules for Cypress right now. We are using the plugin only for the globals. We are overriding the rules as you can see here.
// Linting for Cypress (https://www.npmjs.com/package/eslint-plugin-cypress)
{
files: ['test/cypress/**/*'],
...pluginCypress.configs.recommended,
rules: {
'cypress/no-unnecessary-waiting': 'off'
}
}
I would recommend activating the rules since they follow Cypress best practices. I tested the recommended rules, and we will have a lot to refactor if we decide to activate them.
Here are the plugin's recommended options:
Object.assign(plugin.configs, {
recommended: {
name: 'cypress/recommended',
plugins: {
cypress: plugin
},
rules: {
'cypress/no-assigning-return-values': 'error',
'cypress/no-unnecessary-waiting': 'error',
'cypress/no-async-tests': 'error',
'cypress/unsafe-to-chain-command': 'error',
},
languageOptions: {
globals:
commonGlobals,
...commonLanguageOptions
}
}
})
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 agree we should activate them 👍 let's do it in a separate PR though with the corresponding changes
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 was thinking it would add the rules in addition to ours but I realized it's the config section and not the rules set
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.
Do you want me to move all the linting part to another PR? Should I remove the commit?
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.
Added some comments
} | ||
|
||
// https://docs.cypress.io/app/guides/network-requests#Working-with-GraphQL | ||
export const aliasQuery = (req: IncomingHttpRequest, operation: string, responseBody: unknown) => { |
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.
What is this for? I can't tell from quickly reading it
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.
Set the alias for a request based on the operation name. After setting it, we can use cy.wait(@{alias}).
I'm following the Cypress documentation for working with GraphQL.
test/cypress/utils.ts
Outdated
}) | ||
} | ||
|
||
export const hasOperation = (req: IncomingHttpRequest, operation) => { |
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.
Why do we need this?
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.
Check if the GraphQL operation is included in the request body
test/cypress/utils.ts
Outdated
} | ||
|
||
// https://docs.cypress.io/api/commands/intercept#Interception-lifecycle | ||
export const requestHandler = (req: IncomingHttpRequest) => { |
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 only doing one purpose of clearing the cache, that would be a better function name here
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.
const preventRequestCache = (req: IncomingHttpRequest) => {
req.on('before:response', res => {
// force all API responses to not be cached
res.headers['cache-control'] = 'no-store'
})
}
isInViewport(args?: string): Chainable<JQuery<HTMLElement>> | ||
isNotInViewport(args?: string): Chainable<JQuery<HTMLElement>> | ||
} | ||
declare namespace Cypress{ |
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.
Can this type come from cypress types or an @types/cypress library?
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 are using the Cypress namespace, and we don't have @types/cypress
. We are extending the Cypress namespace with our custom command.
Sorry, I think I didn't understand your question. Could you elaborate more, please?
test/cypress/support/commands.ts
Outdated
autoEnd: false | ||
}) | ||
|
||
log.snapshot('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.
Instead of "before" and "after", can be more explicit like
"connecting to auth0"
And
"Successfully logged in"
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.
cypress: pluginCypress | ||
}, | ||
files: ['test/cypress/**/*'], | ||
...pluginCypress.configs.recommended, |
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.
Please confirm on this one
e7d00a6
to
a3674fc
Compare
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
b4d5b4c
to
8c09bd3
Compare
9bc8279
to
c432760
Compare
- Set the correct Cypress path - Set eslint-plugin-cypress config to recommended Reference: https://www.npmjs.com/package/eslint-plugin-cypress
- Add Cypress environment variables in `cypress.config` - Remove unnecessary support folder configuration - Change e2e support file extension to `.ts` - Update `.gitignore` to ignore `.env.test` Added Yarn support for `.env.test` to securely manage environment variables required for testing. The `.env.test` file includes: - `AUTH0_DOMAIN` - `AUTH0_USERNAME` - `AUTH0_PASSWORD` These variables are critical for the Auth0 login process. To ensure security, only authorized individuals will have access to the `.env.test` file, and it has been added to `.gitignore` to prevent accidental commits of sensitive information.
- Introduce a utils file with the following functions: - `auth0Login`: Handles the Auth0 login routine - `hasOperation`: Checks the request query for a specific operation - `aliasQuery`: Adds an alias for an operation and stubs it - Change file extensions to `.ts` - Add Cypress plugin types to `tsconfig.json` - Import Cypress plugins in `e2e.ts` This commit introduces a custom `cy.login()` command for Cypress that uses the Auth0 login routine to create a login session. This optimizes testing by reducing the need for repetitive logins during tests. Additionally, it adds utility functions to simplify the manipulation of GraphQL requests. Furthermore, Cypress plugins are now imported in a centralized `e2e.ts` file, eliminating the need to import them individually in each test file. References: https://docs.cypress.io/app/guides/network-requests#Working-with-GraphQL https://docs.cypress.io/app/guides/authentication-testing/auth0-authentication https://github.com/dmtrKovalenko/cypress-real-events#readme https://www.npmjs.com/package/cypress-plugin-tab
- Remove unnecessary plugin imports - Directly import JSON fake data - Use the new `cy.login()` command - Separate each test suite into a different spec file This refactor streamlines the login process for tests, improving both speed and efficiency. Instead of using fixtures for fake data, JSON files are now directly imported, as fixtures are unnecessary due to how GraphQL requests are manipulated. Cypress plugin imports have been centralized in the support file, removing the need for redundant imports in individual test files. Additionally, test suites have been separated into different spec files for better organization and maintainability.
- Remove the `--record` and `--key` parameters Updated the e2e tests to use the default browser instead of Chrome. Chrome requires manual permissions for certain actions, such as copying text, which can disrupt automated testing. This change ensures a smoother and more reliable testing process. Additionally, removing the `--record` parameter restores the runner UI in screenshots. This is essential for debugging issues in CLI mode, as the UI provides critical context for test failures.
Updated the configuration to ensure GitHub Actions runs e2e tests using our custom command (`yarn test:e2e`). This guarantees that the tests use our defined configurations, providing consistency between local development and CI environments.
Co-authored-by: Cinnamoroll <[email protected]>
- Add `dotenv-cli` to load environment variables from `.env` files - Add a new package script: - `test:e2e:open` - Opens the Cypress UI with `.env.test` variables Introduced `dotenv-cli` to simplify loading environment variables for tests. Due to `dotenv-cli` loading the env variables only after `--`, additional scripts were added to ensure proper loading. - Use `yarn test:e2e` and `yarn test:e2e:open` for running tests. - Other scripts are included only for debugging and running Cypress commands. - These commands will not work without the `.env.test` file.
c432760
to
989417a
Compare
819a841
to
a54a4ec
Compare
Report too large to display inline |
Resolves #797
🔧 What changed
✨ Linting
Fixed Cypress linting
🧪 Test Environment
Added Yarn support for
.env.test
to securely manage environmentvariables required for testing. The
.env.test
file includes:AUTH0_DOMAIN
AUTH0_USERNAME
AUTH0_PASSWORD
These variables are critical for the Auth0 login process. To ensure
security, only authorized individuals will have access to the
.env.test
file, and it has been added to.gitignore
to preventaccidental commits of sensitive information.
🤖 Cypress
Add Cypress cy.login() custom command
auth0Login
: Handles the Auth0 login routinehasOperation
: Checks the request query for a specific operationaliasQuery
: Adds an alias for an operation and stubs it.ts
tsconfig.json
e2e.ts
This introduces a custom
cy.login()
command for Cypress thatuses the Auth0 login routine to create a login session. This optimizes
testing by reducing the need for repetitive logins during tests.
Additionally, it adds utility functions to simplify the manipulation of
GraphQL requests. Furthermore, Cypress plugins are now imported in a
centralized
e2e.ts
file, eliminating the need to import themindividually in each test file.
📝 Cypress Tests
Simplify login process and enhance cypress.intercept usage
cy.login()
commandThis refactor streamlines the login process for tests, improving both
speed and efficiency. Instead of using fixtures for fake data, JSON
files are now directly imported, as fixtures are unnecessary due to how
GraphQL requests are manipulated.
Cypress plugin imports have been centralized in the support file,
removing the need for redundant imports in individual test files.
Additionally, test suites have been separated into different spec files
for better organization and maintainability.
🎬 GitHub Actions
Updated the configuration to ensure GitHub Actions runs E2E tests
using our custom command (
yarn test:e2e
) and GitHub secrets.This guarantees that the tests follow our defined configurations,
ensuring consistency between local development and CI environments.
📑 References:
https://www.npmjs.com/package/eslint-plugin-cypress
https://www.npmjs.com/package/cypress-plugin-tab
https://docs.cypress.io/app/guides/network-requests#Working-with-GraphQL
https://docs.cypress.io/app/guides/authentication-testing/auth0-authentication
https://github.com/dmtrKovalenko/cypress-real-events#readme
https://github.com/cypress-io/github-action