Add AppVeyor CI support for Windows testing. #1280

XhmikosR · 2016-12-23T18:29:56Z

Preview: https://ci.appveyor.com/project/XhmikosR/lighthouse/history

Before merging: one of the project maintainers needs to add AppVeyor to the repository and paste the image link so that I add it in readme.md and rebase.

wardpeet · 2016-12-27T19:04:38Z

@XhmikosR the variable isn't correctly set because mocha is not running as harmony.
Try to get appveyor working in node 6 / 7 first ;)

I don't have much time to look at it today or tomorrow.

XhmikosR · 2016-12-27T19:11:34Z

Cool thanks. I meant to ask you about this. If you really want node 4 support on Windows or not.

…

On Dec 27, 2016 21:04, "Ward Peeters" ***@***.***> wrote: @XhmikosR <https://github.com/XhmikosR> the variable isn't correctly set because mocha is not running as harmony. Try to get appveyor working in node 6 / 7 first ;) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1280 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVVtcxjDstk2g6wB9AF-Lc2nFYoxHHRks5rMWFIgaJpZM4LU_0c> .

XhmikosR · 2016-12-27T19:48:10Z

@wardpeet: here is my findings so far.

__node_harmony is problematic; it's also in npm scripts which you reference it as $__node_harmony which doesn't work on Windows

npm run build fails for the extension with this

C:\projects\lighthouse>pushd lighthouse-extension && npm run build && popd

> lighthouse@ build C:\projects\lighthouse\lighthouse-extension
> gulp build

[19:42:57] Using gulpfile C:\projects\lighthouse\lighthouse-extension\gulpfile.js
[19:42:57] Starting 'build'...
[19:42:57] Starting 'lint'...
[19:42:57] Finished 'lint' after 431 ms
[19:42:57] Starting 'browserify'...
[19:42:57] Starting 'browserify-lighthouse'...
stream.js:74
      throw er; // Unhandled stream error in pipe.
      ^

Error: Cannot find module 'debug/browser' from 'C:\projects\lighthouse\lighthouse-core\lib'
    at C:\projects\lighthouse\lighthouse-extension\node_modules\browser-resolve\node_modules\resolve\lib\async.js:46:17
    at process (C:\projects\lighthouse\lighthouse-extension\node_modules\browser-resolve\node_modules\resolve\lib\async.js:173:43)
    at ondir (C:\projects\lighthouse\lighthouse-extension\node_modules\browser-resolve\node_modules\resolve\lib\async.js:188:17)
    at load (C:\projects\lighthouse\lighthouse-extension\node_modules\browser-resolve\node_modules\resolve\lib\async.js:69:43)
    at onex (C:\projects\lighthouse\lighthouse-extension\node_modules\browser-resolve\node_modules\resolve\lib\async.js:92:31)
    at C:\projects\lighthouse\lighthouse-extension\node_modules\browser-resolve\node_modules\resolve\lib\async.js:22:47
    at FSReqWrap.oncomplete (fs.js:82:15)

So, how do you want to proceed?

wardpeet · 2016-12-28T05:57:15Z

The pr that fixes debug is not merged yet.
To just test if appveyor is working you could force package.json to load debug 2.2.0 instead of 2.5.x

"debug": "2.2.0",

paulirish · 2017-01-04T03:59:36Z

@wardpeet

The pr that fixes debug is not merged yet.

what PR you talking about? on debug or here?

XhmikosR · 2017-01-04T08:42:00Z

So, I decided to try and convert the relevant scripts to JS; I started with run-mocha.sh. I don't get the failure though so if anyone has any ideas, let me know.

XhmikosR · 2017-01-04T08:44:13Z

Hmm, and the build is erroneously marked as passing in Travis so that's something that should be fixed in another PR.

wardpeet · 2017-01-04T16:09:30Z

@paulirish it is now #1272
@XhmikosR will make time to look at appveyor today :)

brendankenny · 2017-01-04T18:19:45Z

Hmm, and the build is erroneously marked as passing in Travis so that's something that should be fixed in another PR.

It looks like the run-mocha.js script is absorbing the exit code, so an error in the unit tests isn't translating to an error exit code. Can mocha be used as a module instead of in a child process? Alternately you can just exit on any non-zero exit code from the mocha child process.

The error just seems to be the test filename pattern isn't working, so that will need to be tweaked

XhmikosR · 2017-01-04T19:27:08Z

@brendankenny: it seems we'll need the glob package for wildcards to work.

I'm not aware of another way to use mocha.

So after all this, I'm thinking, how about using shell.js which is extensively used in the wild? It should work on all platforms just fine and ease things a lot.

brendankenny · 2017-01-04T19:44:32Z

it seems we'll need the glob package for wildcards to work

since MSYS is installed, can we just keep using the same find call we had before?

XhmikosR · 2017-01-04T19:46:17Z

Well, MSYS isn't a native part of Windows... And even on Windows 10, one has to manually enable Ubuntu.

That is why I suggest shell.js. It should cover all use cases regardless of the OS.

wardpeet · 2017-01-04T21:18:52Z

@XhmikosR lets stick with bash. If you use bash for everything appveyor will work until gulp build. I'm looking into why it's not building as we speak :)

I'm ok for improving the test code later on with powershell support but for now we just want it working on appveyor 💯

XhmikosR · 2017-01-04T21:51:42Z

I believe you are missing the point that you are certainly not helping Windows users by sticking with bash. But anyway even with bash as long as everything works.

…

On Jan 4, 2017 23:18, "Ward Peeters" ***@***.***> wrote: @XhmikosR <https://github.com/XhmikosR> lets stick with bash. If you use bash for everything appveyor will work until gulp build. I'm looking into why it's not building as we speak :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1280 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVVtRA-zhfrHCayN84AJXcDdxuUxa1_ks5rPAy_gaJpZM4LU_0c> .

paulirish · 2017-01-04T23:14:34Z

I'm looking at https://github.com/eslint/eslint/blob/master/Makefile.js which they run on appveyor. It appears they're just passing globs right into the mocha call. Is that possible?

Here's some other folks potentially solving this too: Search · win32 mocha.cmd process CI

wardpeet · 2017-01-05T05:48:54Z

@XhmikosR you are right but first we want appveyor to work so we are sure we are green until a new pr comes in where we can go windows specific! Thanks for all the effort so far!!

@paulirish looks like the mocha.js is just some sugar on top of _mocha which does the same thing as what they are doing in there makefiles, js files.

XhmikosR · 2017-01-11T09:09:49Z

@wardpeet @paulirish @brendankenny: so, I fixed all issues and tests run and pass on my Windows machine with bash (see https://gist.github.com/XhmikosR/1f28e45feb3a2e55e1d6a491402c805a).

On AppVeyor they fail, but I guess it's because they use old Chrome or something. See https://ci.appveyor.com/project/XhmikosR/lighthouse/build/41

Can you sort #1280 (comment)?

After this is merged, we should really find a purely cross platform solution without the need for bash. Maybe pure node.js scripts or shelljs.

XhmikosR · 2017-01-24T22:35:12Z

So we are down to 2 issues for this after #1519.

XhmikosR · 2017-01-26T04:36:27Z

OK, so @wardpeet @paulirish @wardpeet: this works fine now minus the Chrome 56 tests which would work if #1472 was sorted.

Can one of you:

Before merging: one of the project maintainers needs to add AppVeyor to the repository and paste the image link so that I add it in readme.md and rebase.

Also, make sure you enable the option to "Skip branches without appveyor.yml".

XhmikosR · 2017-01-29T10:19:25Z

And SUCCESS! 🕺 See https://ci.appveyor.com/project/XhmikosR/lighthouse/history

Now, we only need to merge #1572 in order to fix #1472, and then one of the maintainers needs to:

Before merging: one of the project maintainers needs to add AppVeyor to the repository and paste the image link so that I add it in readme.md and rebase.

Some final notes:

not sure if we should cache the downloaded Chromium or not in order to match Travis CI behavior; AppVeyor doesn't offer the ability to clean cache via the Web (yet)
tests fail with stable Chrome 56 which is something that CIs can't catch since we always use the latest Chromium; see Current master tests fail with Chrome 56.0.2924.76 (stable) #1573

XhmikosR · 2017-02-02T08:17:54Z

@addyosmani: ping since you originally asked for this :)

Only the above comments are left for this.

brendankenny · 2017-02-10T21:09:10Z

with 1.5 out the door, excited to get this going!

XhmikosR · 2017-02-10T21:11:29Z

Good to hear that :) Please check my comment above and let me know. Also if tou agree with using the same script to get Chromium or not.

…

On Feb 10, 2017 11:09 PM, "Brendan Kenny" ***@***.***> wrote: with 1.5 out the door, excited to get this going! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1280 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVVtbpk8pXDEkornmji11xn07R_M6fpks5rbNH6gaJpZM4LU_0c> .

brendankenny · 2017-02-10T21:14:23Z

I don't believe I have the ability to add AppVeyor with this repo, so @paulirish is going to try it out.

re:caching, we added it in Travis since it's definitely a drag to have to wait for it to be downloaded for every run of the tests, especially if you're tweaking quickly (or worse, if you're trying to debug an issue that only appears when run in the CI :).

OTOH, yeah, if we can't clear the cache that will be an issue as new Chrome versions come out.

XhmikosR · 2017-02-10T23:39:50Z

All right so caching Chromium disabled we really ok. Adding AppVeyor to the report I'll wait for Paul. Just make sure you enable skip branches without appveyor.yml. Then the only thing left is if we should use the same script to download Chromium or not. Otherwise tests work fine and are passing after all the fixes I have submitted all this time :)

…

On Feb 10, 2017 23:14, "Brendan Kenny" ***@***.***> wrote: I don't believe I have the ability to add AppVeyor with this repo, so @paulirish <https://github.com/paulirish> is going to try it out. re:caching, we added it in Travis since it's definitely a drag to have to wait for it to be downloaded for every run of the tests, especially if you're tweaking quickly (or worse, if you're trying to debug an issue that only appears when run in the CI :). OTOH, yeah, if we can't clear the cache that will be an issue as new Chrome versions come out. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1280 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVVtQ-uH61_EryisAp8SpN6ijJgAMz-ks5rbNMzgaJpZM4LU_0c> .

paulirish · 2017-02-11T00:11:38Z

@XhmikosR I think this is the badge URL for ya:

[![Build status](https://ci.appveyor.com/api/projects/status/3bdm5qn9r32ha5cg?svg=true)](https://ci.appveyor.com/project/paulirish/lighthouse)

these were the choices, FYI.

The appveyor project is here: https://ci.appveyor.com/project/paulirish/lighthouse
I've added the appropriate people to the account, so we should all have access to change settings, etc.

XhmikosR · 2017-02-11T00:46:23Z

@paulirish: rebased and added your account's badge.

The AppVeyor settings look good to me. We could rename appveyor.yml to .apppveyor.yml if you prefer that.

The only thing left is if we should use the same script to download Chromium or not for AppVeyor.

wget that ships with MSYS needs `--no-check-certificate` otherwise it fails for SNI. Also, specified `-q` so that the output isn't bloated.

paulirish

lookin good! Excited for this.

paulirish · 2017-02-11T23:11:23Z

This was a long effort, but it's totally worth it. So excited to have test coverage on windows, finally. :)

✅ asserted true: Awesome PR
✅ asserted true: Great collaboration
✅ asserted true: Test coverage upgrade
✅ asserted true: PR merged
4 of 4 tests pass.

XhmikosR · 2017-02-11T23:13:50Z

If it weren't for this I wouldn't have noticed most of the issues so far.

I hate that most devs ignore Windows when it has so big market share ;)

Now if we could sort #1691 eventually, it would make my and all Windows users lives easier. :)

ebidel added the windows label Dec 23, 2016

XhmikosR mentioned this pull request Jan 14, 2017

Windows: Use LIGHTHOUSE_CHROMIUM_PATH if set #1472

Closed

XhmikosR changed the title **WIP** Add AppVeyor CI support for Windows testing. Add AppVeyor CI support for Windows testing. Jan 28, 2017

This was referenced Jan 30, 2017

Current master tests fail with Chrome 56.0.2924.76 (stable) #1573

Closed

Windows: Look into LIGHTHOUSE_CHROMIUM_PATH variable too. #1572

Merged

XhmikosR mentioned this pull request Feb 11, 2017

Switch to cross platofrm scripts for tests #1691

Closed

XhmikosR added 2 commits February 11, 2017 10:19

Add AppVeyor CI support for Windows testing.

350b064

Use download-chrome.sh to download Chrome on Windows too.

49fdcbb

wget that ships with MSYS needs `--no-check-certificate` otherwise it fails for SNI. Also, specified `-q` so that the output isn't bloated.

paulirish approved these changes Feb 11, 2017

View reviewed changes

paulirish merged commit f17f5f8 into GoogleChrome:master Feb 11, 2017

XhmikosR deleted the appveyor branch February 11, 2017 23:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AppVeyor CI support for Windows testing. #1280

Add AppVeyor CI support for Windows testing. #1280

XhmikosR commented Dec 23, 2016 •

edited

Loading

wardpeet commented Dec 27, 2016 •

edited

Loading

XhmikosR commented Dec 27, 2016 via email •

edited

Loading

XhmikosR commented Dec 27, 2016

wardpeet commented Dec 28, 2016

paulirish commented Jan 4, 2017

XhmikosR commented Jan 4, 2017 •

edited

Loading

XhmikosR commented Jan 4, 2017

wardpeet commented Jan 4, 2017

brendankenny commented Jan 4, 2017 •

edited

Loading

XhmikosR commented Jan 4, 2017

brendankenny commented Jan 4, 2017

XhmikosR commented Jan 4, 2017

wardpeet commented Jan 4, 2017 •

edited

Loading

XhmikosR commented Jan 4, 2017 via email

paulirish commented Jan 4, 2017

wardpeet commented Jan 5, 2017

XhmikosR commented Jan 11, 2017 •

edited

Loading

XhmikosR commented Jan 24, 2017

XhmikosR commented Jan 26, 2017

XhmikosR commented Jan 29, 2017 •

edited

Loading

XhmikosR commented Feb 2, 2017

brendankenny commented Feb 10, 2017

XhmikosR commented Feb 10, 2017 via email

brendankenny commented Feb 10, 2017

XhmikosR commented Feb 10, 2017 via email

paulirish commented Feb 11, 2017 •

edited

Loading

XhmikosR commented Feb 11, 2017 •

edited

Loading

paulirish left a comment

paulirish commented Feb 11, 2017

XhmikosR commented Feb 11, 2017

Add AppVeyor CI support for Windows testing. #1280

Add AppVeyor CI support for Windows testing. #1280

Conversation

XhmikosR commented Dec 23, 2016 • edited Loading

wardpeet commented Dec 27, 2016 • edited Loading

XhmikosR commented Dec 27, 2016 via email • edited Loading

XhmikosR commented Dec 27, 2016

wardpeet commented Dec 28, 2016

paulirish commented Jan 4, 2017

XhmikosR commented Jan 4, 2017 • edited Loading

XhmikosR commented Jan 4, 2017

wardpeet commented Jan 4, 2017

brendankenny commented Jan 4, 2017 • edited Loading

XhmikosR commented Jan 4, 2017

brendankenny commented Jan 4, 2017

XhmikosR commented Jan 4, 2017

wardpeet commented Jan 4, 2017 • edited Loading

XhmikosR commented Jan 4, 2017 via email

paulirish commented Jan 4, 2017

wardpeet commented Jan 5, 2017

XhmikosR commented Jan 11, 2017 • edited Loading

XhmikosR commented Jan 24, 2017

XhmikosR commented Jan 26, 2017

XhmikosR commented Jan 29, 2017 • edited Loading

XhmikosR commented Feb 2, 2017

brendankenny commented Feb 10, 2017

XhmikosR commented Feb 10, 2017 via email

brendankenny commented Feb 10, 2017

XhmikosR commented Feb 10, 2017 via email

paulirish commented Feb 11, 2017 • edited Loading

XhmikosR commented Feb 11, 2017 • edited Loading

paulirish left a comment

Choose a reason for hiding this comment

paulirish commented Feb 11, 2017

XhmikosR commented Feb 11, 2017

XhmikosR commented Dec 23, 2016 •

edited

Loading

wardpeet commented Dec 27, 2016 •

edited

Loading

XhmikosR commented Dec 27, 2016 via email •

edited

Loading

XhmikosR commented Jan 4, 2017 •

edited

Loading

brendankenny commented Jan 4, 2017 •

edited

Loading

wardpeet commented Jan 4, 2017 •

edited

Loading

XhmikosR commented Jan 11, 2017 •

edited

Loading

XhmikosR commented Jan 29, 2017 •

edited

Loading

paulirish commented Feb 11, 2017 •

edited

Loading

XhmikosR commented Feb 11, 2017 •

edited

Loading