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

Add watch mode to tests #1748

Closed
wants to merge 1 commit into from

Conversation

marvinhagemeister
Copy link
Contributor

@marvinhagemeister marvinhagemeister commented Oct 12, 2016

This also fixes process variables like process.platform not being properly restored after a test run because getters have a higher precedence.

Tested on both v0.12.0 and v6.7.0.

@@ -32,6 +32,7 @@
"postinstall": "node scripts/build.js",
"lint": "node_modules/.bin/eslint bin/node-sass lib scripts test",
"test": "node_modules/.bin/mocha test",
"test-watch": "node_modules/.bin/mocha test -w",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this. You can already do this with npm.

npm test -- -w

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, I've removed that

After a test run the process variables did always return test values, because getters in javascript have a higher precedence than the actual value.
@marvinhagemeister
Copy link
Contributor Author

@xzyfer Sorry for not getting back any sooner. Upon revising this PR it turns out that getters are actually not needed at all and you can set the value of a property directly with value via Object.defineProperty. That is because the process variables are only marked as non writable which only disallows assignments via an assignment operator.


describe('binding', function() {
beforeEach(function() {
delete require.cache[sassPath];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think @xzyfer removed this stuff in a recent PR, so I'll let him go through this again

@xzyfer xzyfer closed this in 2ccbf07 Nov 12, 2016
@xzyfer
Copy link
Contributor

xzyfer commented Nov 12, 2016

Thanks @marvinhagemeister I've update the get() to value.

@marvinhagemeister marvinhagemeister deleted the test_watch branch November 13, 2016 05:37
@marvinhagemeister marvinhagemeister restored the test_watch branch November 13, 2016 05:38
marvinhagemeister pushed a commit to marvinhagemeister/node-sass that referenced this pull request Nov 13, 2016
The state leakage means that running mocha in watch mode causes
random failures.

Closes sass#1748
@marvinhagemeister
Copy link
Contributor Author

marvinhagemeister commented Nov 13, 2016

@xzyfer The committed fix is incomplete. The leakage happens because process.platform = prevValue; does nothing. It doesn't assign the previous value because process.platform is a non writable property.

$ node
> process.platform
'darwin'
> process.platform = "whatever"
'whatever'
> process.platform
'darwin'
> 

Non writable properties can only be overwritten with Object.defineProperty:

Object.defineProperty(process, 'platform', {
  value: "whatever",
});

I've rebased the PR against master, but lack the permissions to reopen it.

@xzyfer
Copy link
Contributor

xzyfer commented Nov 13, 2016

Apologies the diff nor the comments were clear on what the issue actually was. Rebasing wont work since these specs have been moved out of api.js into binding.js. Please create a new PR with the fixes on the correct specs.

@marvinhagemeister marvinhagemeister deleted the test_watch branch November 13, 2016 09:54
jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this pull request Apr 7, 2024
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

3 participants