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.

Sign up for GitHub

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

Jump to bottom

Fix for issue #622 (Unable to run tests on Windows) #623

Closed
wants to merge 1 commit into from
Closed

Fix for issue #622 (Unable to run tests on Windows) #623

wants to merge 1 commit into from

Conversation

sebastianhaberey
Copy link

@sebastianhaberey sebastianhaberey commented Aug 9, 2019 •
edited
Loading

Description

Amended several commands in package.json for Windows

Updated travis.yml according to cypress docs (see https://docs.cypress.io/guides/guides/continuous-integration.html#Travis) apparently we're not using Travis anymore

@sebastianhaberey
Copy link
Author

Fixes issue #623.

@sebastianhaberey sebastianhaberey mentioned this pull request Aug 9, 2019
Closed
@jshjohnson
Copy link
Collaborator

You have some conflicts :)

@sebastianhaberey
Copy link
Author

Sorry for that, will try to resolve this come weekend!

@sebastianhaberey
Copy link
Author

Ok I updated this, please don't wait another three months or I will have to do another update in in Feb. 😉

tinovyatkin
tinovyatkin reviewed Oct 27, 2019
View reviewed changes
package.json
"cypress:ci": "cypress run --record --group --ci-build-id $GITHUB_SHA",
"test": "run-s test:unit test:e2e",
"test:unit": "mocha",
"test:unit:watch": "mocha --watch --inspect=5556",
"test:e2e": "run-p --race start cypress:run",
"js:watch": "NODE_ENV=development node server.js",
"js:watch": "cross-env NODE_ENV=development node server.js",
Copy link
Contributor

@tinovyatkin tinovyatkin Oct 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think just can remove NODE_ENV=development everywhere in the package.json. The server.js actually checks NODE_ENV !== 'production', so, undefined will works as well as development here.

PS: If you also can add NODE_ENV: production to the cypress.yml action env as part of removing this - that will be 💯

@tinovyatkin
Copy link
Contributor

tinovyatkin commented Oct 29, 2019 •
edited
Loading

@sebastianhaberey #700 introduced more env, please rebase to current master

@jshjohnson
Copy link
Collaborator

Ok I updated this, please don't wait another three months or I will have to do another update in in Feb. 😉

haha, i'm sorry!! 🤦‍♂

@jshjohnson jshjohnson added the chore Pull request that does not change core code label Oct 30, 2019
@jshjohnson
Copy link
Collaborator

Is this fix still required? (Sorry, I don't use Windows)

@tinovyatkin
Copy link
Contributor

tinovyatkin commented Nov 3, 2019 •
edited
Loading

Is this fix still required? (Sorry, I don't use Windows)

NODE_ENV=development should be removed at all (makes nothing), but NODE_ENV=test is still required and needs a workaround on Windows.

And cypress launch scripts do need to have prefix removed.

jshjohnson
jshjohnson requested changes Nov 18, 2019
View reviewed changes
Copy link
Collaborator

@jshjohnson jshjohnson left a comment

Choose a reason for hiding this comment

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

See comments above

@jshjohnson
Copy link
Collaborator

Superseded by #784 - thanks 👍

@jshjohnson jshjohnson closed this Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tinovyatkin tinovyatkin tinovyatkin left review comments

@jshjohnson jshjohnson jshjohnson requested changes

Assignees
No one assigned
Labels
chore Pull request that does not change core code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sebastianhaberey @jshjohnson @tinovyatkin