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 and fix appveyor #195

Closed
wants to merge 13 commits into from
Closed

Conversation

@codyj110
Copy link
Contributor

codyj110 commented Dec 25, 2019

Updated node versions for appveyor; Everything below node 8 was failing with Object.entries not a function.

I added -u option to appveyor test because snapshots from linux and windows do not play nice it seems. Windows didn't want the expected shell files, and linux does with the test. the u will update the snapshot for the tests. This is def a work around not sure what the best solution is for this problem. Possibly separating snapshots for windows and linux?

updated dif-compare package, and also updated how are calling it to match what documentation they had on npm site.

@codyj110 codyj110 requested a review from jondot Jan 18, 2020
@@ -16,7 +16,7 @@ shallow_clone: true
test_script:
- node --version
- yarn --version
- yarn test:win32
- yarn test:win32 -u

This comment has been minimized.

Copy link
@Whoaa512

Whoaa512 Feb 10, 2020

Contributor

This will break snapshot behavior. Can you update the snapshots locally and commit them?

This comment has been minimized.

Copy link
@codyj110

codyj110 Feb 10, 2020

Author Contributor

As i said in my original comment this is a work around. The test can still fail or pass depending on logic issues, but it will never fail on the snapshot. If you noticed appveyor always fails. That is because of the snapshot diff between linux, and windows. I explained all this in my original comment. If you know of a way to make the snapshot work for both please fix snapshot undo -u and push up to this branch and then we can fix it for good.

This comment has been minimized.

Copy link
@codyj110

codyj110 Feb 10, 2020

Author Contributor

also the node versions were way behind as well and some of the code will not work on the older versions

This comment has been minimized.

Copy link
@codyj110

codyj110 Feb 10, 2020

Author Contributor

I made this pr because i wanted to provide a quick patch to this problem so there is some value in running appveyor. If it is not fixed then it is not helping this project an any way. This does several things 1. people start ignoring test when contributing 2. people looking to use this tool will avoid it because of the failed pipelines 3. confusion for potential contributors.

@codyj110

This comment has been minimized.

Copy link
Contributor Author

codyj110 commented Feb 11, 2020

@jondot what are your thoughts on this?

@codyj110 codyj110 closed this Feb 11, 2020
@codyj110

This comment has been minimized.

Copy link
Contributor Author

codyj110 commented Feb 11, 2020

closing due to inactivity

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.