Skip to content
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

Update the config to delete the recorded video if no tests are retried #98

Merged
merged 2 commits into from
Oct 12, 2022
Merged

Conversation

shyambh
Copy link
Contributor

@shyambh shyambh commented Oct 12, 2022

I have updated the cypress.config.js file to add the code to delete the recorded videos if none of the tests are retried.
I basically simply followed the steps mentioned here:
https://docs.cypress.io/api/plugins/after-spec-api#Delete-the-recorded-video-if-no-tests-retried

@@ -1,7 +1,8 @@
const {defineConfig} = require('cypress');
const {tagify} = require('cypress-tags');
const { defineConfig } = require('cypress');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove the spaces here? The coding standard requires there are no spaces there.

const {defineConfig} = require('cypress');
const {tagify} = require('cypress-tags');
const { defineConfig } = require('cypress');
const { tagify } = require('cypress-tags');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove the spaces here? The coding standard requires there are no spaces there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @peterjaap , can you please take a look now? I have removed the whitespaces.

@peterjaap peterjaap merged commit 60a46a6 into elgentos:main Oct 12, 2022
@shyambh
Copy link
Contributor Author

shyambh commented Oct 12, 2022

Also can you please add 'hactoberfest-accepted' label to this PR if you think it is fine?

Like so:
image

@peterjaap
Copy link
Contributor

@shyambh I didn't know I had to do that, sorry. Done now.

@Vinai
Copy link
Collaborator

Vinai commented Oct 13, 2022

This MR breaks the config with

Your configFile is invalid: ...magento2-cypress-testing-suite/cypress.config.js

It threw an error when required, check the stack trace below:

Error [ERR_REQUIRE_ESM]: require() of ES Module ...magento2-cypress-testing-suite/node_modules/del/index.js from ...magento2-cypress-testing-suite/cypress.config.js not supported.
Instead change the require of index.js in /Volumes/CaseSensitive/Workspace/php-sites/mos-hyva-245/magento2-cypress-testing-suite/cypress.config.js to a dynamic import() which is available in all CommonJS modules.

@Vinai
Copy link
Collaborator

Vinai commented Oct 13, 2022

Adding a dependency for something as simple as deleting files and folders recursively is maybe not necessary.

@shyambh
Copy link
Contributor Author

shyambh commented Oct 13, 2022

@Vinai Thank you for this. I will take a look at this.

@Vinai
Copy link
Collaborator

Vinai commented Oct 13, 2022

Another issue is you use _ (lowbar/underscore), which is not available.

return _.some(test.attempts, { state: 'failed' })

Again, instead of using a dependency, why not use native JS?

return test.attempts.find(({state}) => state === 'failed')

@Vinai
Copy link
Collaborator

Vinai commented Oct 13, 2022

Did you even test the code before submitting the PR @shyambh ?

@shyambh
Copy link
Contributor Author

shyambh commented Oct 13, 2022

@Vinai I really want to apologize for this.

I used a dynamic import for the 'del' package and now the tests are running.

image

Regarding the dependency on the external package, I followed the implementation steps mentioned in the Cypress documentation here:

https://docs.cypress.io/api/plugins/after-spec-api#Delete-the-recorded-video-if-no-tests-retried

Please let me know if I can push the fix?

Vinai added a commit to Vinai/magento2-cypress-testing-suite that referenced this pull request Oct 13, 2022
@Vinai Vinai mentioned this pull request Oct 13, 2022
@Vinai
Copy link
Collaborator

Vinai commented Oct 13, 2022

Don't worry, already opened a PR #100

peterjaap pushed a commit that referenced this pull request Oct 13, 2022
* Fix video cleanup from PR #98

* Remove unused deps from PR 98
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants