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.

Sign up for GitHub

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

Jump to bottom

adds ability to run tests on windows; adds appveyor to CI; closes #1813 #2345

Merged
merged 3 commits into from
Jul 2, 2016
Merged

adds ability to run tests on windows; adds appveyor to CI; closes #1813 #2345

merged 3 commits into from
Jul 2, 2016

Conversation

boneskull
Copy link
Contributor

This is based on @TimothyGu's work in #2270, which was based on #1814.

In 78f36fc, I added appveyor.yml. All test modifications simply increased timeout values, as appveyor seems to run tests slowly.

TimothyGu and others added 2 commits June 28, 2016 22:32
@TimothyGu @boneskull
Support test suite on Windows
8fe36a3 
Based on work by Tyler Waters <tyler.waters@gmail.com> in #1814.

This commit contains the following changes:

- Add quotation marks to Makefile variables for programs. The variables
  use POSIX-style paths, which cannot be used on Windows to launch a
  program except when quoted. Using double quotation marks instead of
  single since the latter is not available on Windows
- Use os-tmpdir module to get an acceptable directory for temporary
  usage instead of relying on the POSIX /tmp
- Use process.execPath as an authoritative path for Node.js executable
- Detect whether symbolic links are supported on the platform before
  testing. On Windows, creating symlinks can fail since it needs
  additional user permissions
- Fix hook tests. The tests parse output of the "dot" reporter to
  separate output of individual tests. The "dot" reporter uses "·"
  symbol (U+2024 ONE DOT LEADER) under POSIX environments and "." symbol
  (U+002E FULL STOP) under Windows, which means that having "." in the
  echoed message makes it ambiguous to be parsed in Windows. To fix this
  issue, two separate changes are necessary:
  - Use a dynamically created regular expression to split the tests
    based on the specific dot character used on the platform
  - Replace "." with "-" in echoed messages in fixtures for hook tests
    to avoid ambiguity with the character output by the reporter

Changes from #1814 include:

- Rebasing
- Use process.execPath as an authoritative path for Node.js executable
- Avoid external dependencies for child_process.spawn()
- Detect whether symbol links are supported on the platform before
  testing. On Windows, creating symlinks can fail since it needs
  additional user permissions

Fixes #1813.
@boneskull
run tests on appveyor; closes #1813
78f36fc
@boneskull
Copy link
Contributor Author

cc @mochajs/core

TimothyGu
TimothyGu reviewed Jul 1, 2016
View reviewed changes
appveyor.yml
- ps: Install-Product node $env:nodejs_version
- set CI=true
- set PATH=%APPDATA%\npm;c:\MinGW\bin;%PATH%
- set PHANTOMJS_CDNURL=https://cnpmjs.org/downloads
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this?

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'll see it's in .travis.yml as well. phantomjs hosts the prebuilts on bitbucket, but bitbucket has terrible rate-limiting, so CI will frequently just die when it encounters a 403.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relevant issue

@TimothyGu
Copy link
Contributor 

  1) pending asynchronous skip() in beforeEach should skip all suite specs:
     Error: timeout of 1000ms exceeded. Ensure the done() callback is being called in this test.
      at null.<anonymous> (C:\projects\mocha\lib\runnable.js:226:19)

LMAO.

@boneskull
Copy link
Contributor Author

Error: timeout of 1000ms exceeded

oh for fuck's sake.

@boneskull
Copy link
Contributor Author

@TimothyGu you wouldn't know of any way to magically make appveyor not take 30 minutes for a build to even leave the queue, would you?

@boneskull
Copy link
Contributor Author

If I had to guess MS is just really stingy with the CPU time for free plans

@TimothyGu
Copy link
Contributor

you wouldn't know of any way to magically make appveyor not take 30 minutes for a build to even leave the queue, would you?

Nope.

If I had to guess MS is just really stingy with the CPU time for free plans

Unfortunately AppVeyor is not a MS product, which is probably why they are so stingy. But yes, they are VERY stingy about CPU time, but EVEN FOR PAID PLANS!!!

image

So basically paying $29.50/month (or $295/yr) gets us "priority technical support," and NOTHING else. 67% more than that, and we get ONE more concurrent job.

I also looked into getting a cloud server just for Windows testing. On AWS a Windows t2.medium (2 CPUs, 4 GB RAM) instance costs at least $52.71/month, and a t2.small (1 CPU, 2 GB RAM) costs $26.36 (+ disk, but that's negligible). Not sure if t2.small has enough RAM though.

On Azure, a basic A1 (1 CPU, 1.75 GB RAM) costs $55.06, and a basic A2 (2 CPUs, 3.5 GB RAM) costs $110.11.

It's just that the Windows OS is so expensive...

@boneskull
Copy link
Contributor Author

boneskull commented Jul 1, 2016 •
edited
Loading

Is it legal to repurpose an "IE" VM for this?

@ScottFreeCode
Copy link
Contributor

At least if we need to go the route of buying private Windows cloud hosting for CI purposes, it'd be a good use of our OpenCollective funds.

(It'd be nice to find something that could run older versions of IE, though, as a workaround to not need SauceLabs to test that and, by extension, being able to test it on PRs.)

ScottFreeCode
ScottFreeCode reviewed Jul 1, 2016
View reviewed changes
appveyor.yml
- npm test
skip_commits:
message: /\[ci\s+skip\]/

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work for Travis too? (And/or is there a way to do it for one and not the other if we ever have a reason to?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[ci skip]? yes.

@ScottFreeCode
Copy link
Contributor

Looks good to me!

@boneskull
Copy link
Contributor Author

At least if we need to go the route of buying private Windows cloud hosting for CI purposes, it'd be a good use of our OpenCollective funds.

I'm not sure I can really get behind that. Yes, it's good to know Mocha at least runs on a Windows box, but given the price of AppVeyor, perhaps we can investigate another solution which is more Node.js and OSS friendly.

@boneskull
Copy link
Contributor Author

It's unclear to me why this particular build is failing. npm complains the SHA of esutils@2.0.2 doesn't match what's expected.

@boneskull
Copy link
Contributor Author

Locally, I can't reproduce any such issue when I use Node.js v0.10, npm v2.15.1, and npm install esutils@2.0.2

@boneskull boneskull merged commit c7aec30 into master Jul 2, 2016
@boneskull boneskull deleted the appveyor branch July 2, 2016 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@boneskull @TimothyGu @ScottFreeCode