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

Update spec runner for full sass-spec coverage #1698

Merged
merged 2 commits into from
Sep 8, 2016

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Sep 5, 2016

  • Update the spec runner from work in sass-spec
  • Bump sass-spec tag to 3.3.6

Previously the runner only picked up the first level of the test suites.
Now we pick up each folder as a Mocha suite, which increases the tests
run from ~1K to 6K.
@nschonni nschonni changed the title [DNM] Update spec runner Update spec runner for full sass-spec coverage Sep 6, 2016
@nschonni
Copy link
Contributor Author

nschonni commented Sep 6, 2016

OK, I've rebase the commits. I think it's ready for review/landing now

@xzyfer
Copy link
Contributor

xzyfer commented Sep 6, 2016

Nice one. I'm too fussed with specifics of the implementation as long as it works. If you're happy with it 🚢

@xzyfer
Copy link
Contributor

xzyfer commented Sep 6, 2016

Related #1705

@xzyfer
Copy link
Contributor

xzyfer commented Sep 7, 2016

Nice one. I had originally cherry-picked this PR into mine. I figured there was value in having them separate incase mine needed to be reverted. 🚢

@nschonni
Copy link
Contributor Author

nschonni commented Sep 7, 2016

Yeah, I wanted to make sure they worked together

@nschonni nschonni modified the milestones: 3.10.0, next.minor Sep 7, 2016
@nschonni
Copy link
Contributor Author

nschonni commented Sep 7, 2016

Think I need to add back in the path replacements for Windows. I'll take a look tomorrow

@nschonni
Copy link
Contributor Author

nschonni commented Sep 7, 2016

Think this should fix the wall of Windows errors, but I'm not getting this

  1) cli node-sass sass/ --output css/ should not error if output directory is a symlink:
     Error: EEXIST: file already exists, mkdir 'C:\Workspaces\node-sass\test\fixtures\input-directory\css'
      at Error (native)
      at Object.fs.mkdirSync (fs.js:794:18)
      at Context.<anonymous> (C:\Workspaces\node-sass\test\cli.js:582:10)
      at callFnAsync (C:\Workspaces\node-sass\node_modules\mocha\lib\runnable.js:349:8)
      at Test.Runnable.run (C:\Workspaces\node-sass\node_modules\mocha\lib\runnable.js:301:7)
      at Runner.runTest (C:\Workspaces\node-sass\node_modules\mocha\lib\runner.js:422:10)
      at C:\Workspaces\node-sass\node_modules\mocha\lib\runner.js:528:12
      at next (C:\Workspaces\node-sass\node_modules\mocha\lib\runner.js:342:14)
      at C:\Workspaces\node-sass\node_modules\mocha\lib\runner.js:352:7
      at next (C:\Workspaces\node-sass\node_modules\mocha\lib\runner.js:284:14)
      at Immediate._onImmediate (C:\Workspaces\node-sass\node_modules\mocha\lib\runner.js:320:5)

  2) cli node-sass --follow --output output-dir input-dir should compile with the --follow option:
     Error: EEXIST: file already exists, mkdir 'C:\Workspaces\node-sass\test\fixtures\follow\input-dir'
      at Error (native)
      at Object.fs.mkdirSync (fs.js:794:18)
      at Context.<anonymous> (C:\Workspaces\node-sass\test\cli.js:623:10)
      at callFnAsync (C:\Workspaces\node-sass\node_modules\mocha\lib\runnable.js:349:8)
      at Test.Runnable.run (C:\Workspaces\node-sass\node_modules\mocha\lib\runnable.js:301:7)
      at Runner.runTest (C:\Workspaces\node-sass\node_modules\mocha\lib\runner.js:422:10)
      at C:\Workspaces\node-sass\node_modules\mocha\lib\runner.js:528:12
      at next (C:\Workspaces\node-sass\node_modules\mocha\lib\runner.js:342:14)
      at C:\Workspaces\node-sass\node_modules\mocha\lib\runner.js:352:7
      at next (C:\Workspaces\node-sass\node_modules\mocha\lib\runner.js:284:14)
      at Immediate._onImmediate (C:\Workspaces\node-sass\node_modules\mocha\lib\runner.js:320:5)

But I may just have something broken somehow when I reset my branch. Appveyor will tell the truth

@xzyfer
Copy link
Contributor

xzyfer commented Sep 8, 2016

@nschonni I believe this is because our tests have side effects i.e. creating files on disk. Not all the tests clean up after themselves. So running tests after a failed run could end up with created files having not been removed.

We need to address this because it can cause issues with the retry added for the timeouts in CI.

@nschonni nschonni merged commit ccfbbcb into sass:master Sep 8, 2016
@nschonni nschonni deleted the update-spec-runner branch September 8, 2016 01:46
@nschonni
Copy link
Contributor Author

nschonni commented Sep 8, 2016

I'll see if I can find out what needs to change in the before/after test to see what might need cleaning up

xzyfer added a commit that referenced this pull request Sep 30, 2016
This originally caused issues because sass spec was in the test
folder. As of #1698 sass spec is now an npm devDependency. Without
the test folder published the node-citgm cannot execute our
`npm test`.
jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this pull request Apr 7, 2024
Should return unquoted strings from function calls in interpolants
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants