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

Move coverage script to Makefile #503

Merged
merged 1 commit into from
Oct 31, 2014
Merged

Move coverage script to Makefile #503

merged 1 commit into from
Oct 31, 2014

Conversation

kevva
Copy link
Member

@kevva kevva commented Oct 31, 2014

Also includes some cleanup of unnecessary files in .gitignore which should belong
in a global .gitignore.

Also includes some cleanup of unnecessary files in .gitignore which should belong
in a global .gitignore.
@am11
Copy link
Contributor

am11 commented Oct 31, 2014

Awesome! 👍


lib-coverage/
sass-coverage.js
**/fixtures/**/build.*
Copy link
Member Author

Choose a reason for hiding this comment

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

This shouldn't be needed either. If that's needed it means that tests are failing/are badly written.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes! (unlink!!) I think it was due to the faulty libsass code during the testing. :)

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 17e11ae on kevva:coverage into * on sass:master*.

@am11
Copy link
Contributor

am11 commented Oct 31, 2014

Perfect! Coverage 95%! 👍

@andrew, LGTM! :shipit:

@kevva
Copy link
Member Author

kevva commented Oct 31, 2014

It's only testing the API though, not the CLI. We can't run the bin folder through jscoverage because the binaries lives there too.

@andrew
Copy link
Contributor

andrew commented Oct 31, 2014

@kevva Can you re-add npm-debug.log to that file? In my experience we can't assume every contributor has a good global .gitignore set up, theres no harm in leaving those entries in there so we don't accidently get those files added in a PR.

@kevva
Copy link
Member Author

kevva commented Oct 31, 2014

@andrew, there's *.log at the top :).

@andrew
Copy link
Contributor

andrew commented Oct 31, 2014

Ah, good spot, code review on mobile is tricky!

andrew added a commit that referenced this pull request Oct 31, 2014
Move coverage script to Makefile
@andrew andrew merged commit 8ac7c03 into sass:master Oct 31, 2014
@am11
Copy link
Contributor

am11 commented Oct 31, 2014

LOL! That's true. Most honest mistakes transpire due to mobile technology.

@nschonni
Copy link
Contributor

nschonni commented Nov 1, 2014

👎 Makefiles don't work on Windows, this is why we had the NPM and node scripts

@kevva
Copy link
Member Author

kevva commented Nov 1, 2014

This is only for the coverage stuff which is used by Travis anyway. That's why I kept make test out of the npm test script.

Another solution is to move the whole test suite to gulp or something. Anything that's more reliable and pretty than writing shell scripts using shelljs.

@nschonni
Copy link
Contributor

nschonni commented Nov 1, 2014

The coverage can also be setup on Windows with AppVeyor, but not if we use a Makefile.
Sorry if I'm coming off as a jerk, but now we have to maintain a Makefile + the NPM scripts for no apparent benefit.

@kevva
Copy link
Member Author

kevva commented Nov 1, 2014

No problem. I'm kinda agreeing with you. It's not ideal to have both those scripts and a Makefile. But we should aim to replace those scripts with something better. The Makefile could work on Windows if we used nmake. As an alternative I'm up for using gulp.

@nschonni
Copy link
Contributor

nschonni commented Nov 1, 2014

Yeah, I didn't do Grunt/Gulp before because there was no pressing need at the time. Now that there are more people working, it might be good for that to be revisited.

@andrew
Copy link
Contributor

andrew commented Nov 1, 2014

I'm still not a fan of grunt/gulp, npm scripts solves things in a simple way.

I completely forgot about makefiles on windows too 😦

@keithamus
Copy link
Member

👍 for using npm scripts over grunt (http://blog.keithcirkel.co.uk/why-we-should-stop-using-grunt/)

@kevva
Copy link
Member Author

kevva commented Nov 3, 2014

+1 for using anything over grunt. That article got so many facts wrong about gulp though and shouldn't be treated as an argument for stop using it (not saying we should use gulp here though).

jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this pull request Apr 7, 2024
Fixes build to report correctly to coveralls.io
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.

None yet

6 participants