Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Remove Ruby dependency from test #1686

Closed
xzyfer opened this issue Aug 29, 2016 · 3 comments
Closed

Remove Ruby dependency from test #1686

xzyfer opened this issue Aug 29, 2016 · 3 comments

Comments

@xzyfer
Copy link
Contributor

xzyfer commented Aug 29, 2016

Background

We run sass-spec as part of npm test along side our own tests. This is important because the C++ bridge code can result in some quirks. These tests have, and continue to high light potential issues.

Currently we do this by git sub-moduling sass-spec into node-sass. We have a small script that runs the specs against node-sass. This has been possible because sass-spec has, until recently, been targeted at LibSass only.

note: we currently do not publish these specs, but that's an oversight that will be corrected.

The problem

@chriseppstein has been hard at work improving the sass-spec CLI to handle both LibSass and Sass. Part of these improvements has been the addition of annotations. These annotations enable running a subset of specs for instance: "all spec for Sass 3.4 that LibSass supports".

An unfortunate side-effect of annotation is that ruby is required to target the correct subset of specs for LibSass with the sass-spec CLI. Although this is OK for our travis environments, and makes creates a burden on contributors to run our tests. Could also be an issue for AppVeyor, but I'm not confident enough to say for sure.

This has recently come to light because node citgm is attempting to integrate node-sass. This is something that is going to really valuable. Currently they are blocked on us not publishing our spec. This is something I'll address shortly. This is possible because we're pinned to an older version of sass-spec.

Moving forward

We need to get sass-spec annotation support without the Ruby dependency.

As a work around, we have not updated our sass-spec submodule for a couple releases. This is a temporary measure and will not be tenable as we start to rollout betas with 3.5 feature support.

I can see a couple options.

Read-only sass-spec tool in Node

A slimmed down, annotation aware CLI in Node. This allows us to remove the Ruby dependency from CI, and the development workflow.

Add an ls command to sass-spec

Add a CLI command to list the absolute paths for specs that match the current query i.e. impl, start, end.

This doesn't remove the Ruby dependency from our CI. It will allow us to publish the appropriate subset of specs for the citgm project.

This could open up some alternatives to the submodule i.e. committing the relevant specs to node-sass directly.

Other options?

I'm open to other options.

IMHO not running sass-spec isn't an option, especially as we're trying to add support for more runtimes (#1630).

Would love to hear your thoughts @saper @nschonni @chriseppstein @nex3.

@xzyfer
Copy link
Contributor Author

xzyfer commented Aug 29, 2016

Publishing test addressed in #1688

@nschonni
Copy link
Contributor

I was bored so I tried the read-only Node tool with sass/sass-spec#898. Not quite there yet, but I can finish it up if it looks like it's on the right track.

@xzyfer xzyfer changed the title [RFC] Remove Ruby dependency from test Remove Ruby dependency from test Sep 6, 2016
@xzyfer
Copy link
Contributor Author

xzyfer commented Sep 8, 2016

This is done 🎉 awesome work @nschonni

@xzyfer xzyfer closed this as completed Sep 8, 2016
@xzyfer xzyfer added this to the 3.10.0 milestone Sep 8, 2016
jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this issue Apr 7, 2024
Don't throw too many arguments error is there is a rest parameter
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants