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

add retry-ability #128

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

add retry-ability #128

wants to merge 16 commits into from

Conversation

0xIslamTaha
Copy link

As a retry-ability is a core function of Cypress, I tried to make it works with minimum changes.

@0xIslamTaha 0xIslamTaha mentioned this pull request Apr 14, 2020
exports.matchImageSnapshotPlugin = matchImageSnapshotPlugin;
exports.addMatchImageSnapshotPlugin = addMatchImageSnapshotPlugin;

var _fsExtra = require('fs-extra');

Choose a reason for hiding this comment

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

why not const? also _ means private for me, and this is not a private variable. I would const fsExtra = require('fs-extra');

Choose a reason for hiding this comment

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

also, other files in the project use import syntax. plz support if possible.
ref: https://github.com/palmerhq/cypress-image-snapshot/blob/master/src/plugin.js

Copy link
Author

Choose a reason for hiding this comment

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

why not const? also _ means private for me, and this is not a private variable. I would const fsExtra = require('fs-extra');

this is the same way of coding in the same file

src/command.js Outdated
@@ -43,6 +43,11 @@ export function matchImageSnapshotCommand(defaultOptions) {
diffOutputPath,
}) => {
if (!pass && !added && !updated) {
if ((commandOptions.retryCounter || 0) > 0) {
cy.wait(100);

Choose a reason for hiding this comment

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

this 100 ms is fine as a default but it should be one of the options and be configurable

Copy link
Author

Choose a reason for hiding this comment

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

done

src/command.js Outdated
@@ -43,6 +43,11 @@ export function matchImageSnapshotCommand(defaultOptions) {
diffOutputPath,
}) => {
if (!pass && !added && !updated) {
if ((commandOptions.retryCounter || 0) > 0) {

Choose a reason for hiding this comment

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

this can be simplified to
if (commandOptions.retryCounter)

Copy link
Author

Choose a reason for hiding this comment

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

done

command.js Outdated
diffOutputPath,
}) => {
if (!pass && !added && !updated) {
if ((commandOptions.retryCounter || 0) > 0) {

Choose a reason for hiding this comment

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

u can simply if (commandOptions.retryCounter)...same comment on configurability of 100 as elsewhere also

Copy link
Author

Choose a reason for hiding this comment

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

done

README.md Outdated
@@ -161,6 +161,8 @@ or add the following to your `cypress.json`
- `customSnapshotsDir` : Path to the directory that snapshot images will be written to, defaults to `<rootDir>/cypress/snapshots`.
- `customDiffDir`: Path to the directory that diff images will be written to, defaults to a sibling `__diff_output__` directory alongside each snapshot.

- `retryCounter` : Retry up to n times before raise a failer and the execution gonna wait for 100 millisecond betwen each iteration.
Copy link

@Vandivier Vandivier Apr 15, 2020

Choose a reason for hiding this comment

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

slightly better grammar:
Retry up to n times before raising a failure. The process will wait for 100 milliseconds between each iteration.

and, if 100 becomes a configurable default instead of a constant, insert "by default" before final period

Copy link
Author

Choose a reason for hiding this comment

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

done

@Vandivier
Copy link

lgtm

@changeset-bot
Copy link

changeset-bot bot commented Jun 5, 2021

⚠️ No Changeset found

Latest commit: c547698

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants