-
Notifications
You must be signed in to change notification settings - Fork 511
Conversation
-1 from me, one CI is enough, i think it's fair to say we depend on other tools for jscs to be cross-platform. |
Alright. jshint repo has it, I thought it might be a good idea to configure appveyor for jscs as well (especially for curious contributors, who are not using Windows machine). |
@mdevils what is your take on this? |
FWIW, I don't think it can really hurt. |
ping @mdevils |
+1 from me. We had issues with windows and several times we broke windows. |
In this example tests were not run, just |
56e6ad0
to
c167c0a
Compare
21cff41
to
9d24239
Compare
@mdevils I have enabled the tests. Here are some notes:
Meanwhile I have marked https://ci.appveyor.com/project/am11/node-jscs/build/20 @dougwilson, @FeodorFitsner, is it normal to get |
"rewire": "~2.1.0", | ||
"sinon": "~1.12.0", | ||
"rewire": "~2.1.4", | ||
"sinon": "~1.12.2", |
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.
Is dependencies update required?
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.
Nops, just did it in a separate commit. All is working fine though.
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.
Alrighty, it's fine, could merge it with it
@@ -1,4 +1,5 @@ | |||
[![Build Status](https://travis-ci.org/jscs-dev/node-jscs.svg?branch=master)](https://travis-ci.org/jscs-dev/node-jscs) | |||
[![Build Status](https://travis-ci.org/jscs-dev/node-jscs.svg?branch=master)](https://travis-ci.org/jscs-dev/node-jscs) |
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 is this line changed? Seems no different to me? @am11, are you using correct line endings?
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.
@mikesherov, yeah that was both CRLF and BOM. Fixed by 3477d8a.
test_script: | ||
- npm test | ||
|
||
build: off |
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 you give me link to this setting?
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.
Sure: http://www.appveyor.com/docs/appveyor-yml (Ctrl+F, search for: '174')
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.
They use build
slug for msbuild systems (mostly Visual Studio projects).
@mikesherov, done. :) |
LGTM |
This PR:
It is passing all the tests: https://ci.appveyor.com/project/am11/node-jscs/build/6.
Previously, for some crazy reason, it was failing with node version 0.11.14, so I removed version matrix from the configuration which renders it test only on latest stable version of node (v0.10.35). On TravisCI, jscs is also tested against node v0.10.35, so I presume the issue is common (probably due to some dependency?).See #937 (comment).
Next steps:
jscs-dev
from dropdown list (label: Account) and press the GitHub button again.🎉