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

Instructions in testing guide fail to run client tests from the command line #8225

Closed
danstiner opened this Issue Jan 7, 2017 · 16 comments

Comments

Projects
None yet
5 participants
@danstiner

danstiner commented Jan 7, 2017

While following the instructions in the testing guide for configuring command line execution using PhantomJS (https://guide.meteor.com/testing.html) I consistently see the error message cannot execute binary file when running client tests. This seems to be because the phantomjs being installed is for MacOS but I am running Linux x64.

Steps to reproduce on a Linux machine:

curl https://install.meteor.com/ | sh
meteor create phantomjs-tests
cd phantomjs-tests/
meteor npm install --save babel-runtime
meteor add dispatch:mocha-phantomjs
meteor test --once --driver-package dispatch:mocha-phantomjs

Resulting output:

--------------------------------
----- RUNNING CLIENT TESTS -----
--------------------------------

/home/dan/.meteor/packages/dispatch_phantomjs-tests/.0.0.7.1k3403j++os+web.browser+web.cordova/npm/node_modules/phantomjs-prebuilt/lib/phantom/bin/phantomjs: /home/dan/.meteor/packages/dispatch_phantomjs-tests/.0.0.7.1k3403j++os+web.browser+web.cordova/npm/node_modules/phantomjs-prebuilt/lib/phantom/bin/phantomjs: cannot execute binary file

Running file on the binary indicates it is actually a Mach-O executable meant for MacOS:

$ file /home/dan/.meteor/packages/dispatch_phantomjs-tests/.0.0.7.1k3403j++os+web.browser+web.cordova/npm/node_modules/phantomjs-prebuilt/lib/phantom/bin/phantomjs
Mach-O 64-bit x86_64 executable, flags:<NOUNDEFS|DYLDLINK|TWOLEVEL|BINDS_TO_WEAK|PIE>

This can be worked around by running the following, which leads me to believe this is an issue with meteor not correctly doing the rebuild, and that this is not an issue with the phantomjs-tests package.

cd /home/dan/.meteor/packages/dispatch_phantomjs-tests/.0.0.7.1k3403j++os+web.browser+web.cordova/npm
npm rebuild

Related discussion on DispatchMe/meteor-mocha-phantomjs#35

Meteor 1.4.2.3
dispatch:mocha-phantomjs@0.1.9
dispatch:phantomjs-tests@0.0.7
x86_64 GNU/Linux

@aldeed

This comment has been minimized.

Contributor

aldeed commented Jan 9, 2017

@benjamn I have been getting a lot of complaints about this. All I did was publish dispatch:phantomjs-tests with a couple small changes from Mac, but it was done on latest Meteor for the first time so this effectively caused a breaking change in a non-breaking version. It seems that meteor npm rebuild, as suggested in https://forums.meteor.com/t/whats-happening-to-meteor-publish-for-arch-in-meteor-1-4-1/28209 and the guide, does not actually rebuild the non-local cached packages, or maybe they are overwritten after being rebuilt. I haven't investigated much but I did run into this myself on Travis.

I thought maybe I could fix by doing publish-for-arch anyway, but it would not let me.

@abernix

This comment has been minimized.

Member

abernix commented Jan 16, 2017

@aldeed Just out of curiosity, which version of Meteor were you previously publishing dispatch:phantomjs-tests from? I'll try to take a look at this today.

@abernix abernix self-assigned this Jan 16, 2017

@aldeed

This comment has been minimized.

Contributor

aldeed commented Jan 24, 2017

@abernix I think 1.3.x? Not sure exactly since there was a long period between publishing the last working version and the patch that started breaking things.

@aldeed

This comment has been minimized.

Contributor

aldeed commented Jan 24, 2017

@abernix There may be other info helpful for you to investigate in the linked issue DispatchMe/meteor-mocha-phantomjs#35

@danstiner

This comment has been minimized.

danstiner commented Jan 25, 2017

After some digging I have found what I think to be the issue. Pardon my unfamiliarity with meteor internals.

In commit 861880b the behavior of meteorNpm.rebuildIfNonPortable was changed by @benjamn from looking at a packages' npm modules and rebuilding all of them if any was non-portable, to a more "aggressive" approach of only rebuilding individual non-portable npm modules.

The old behavior worked because dispatch:phantomjs-tests has the module meteor-force-non-portable which would force a rebuild of all modules under ~/.meteor/packages/dispatch_phantomjs-tests/0.0.7/npm/node_modules. Under the new behavior only that specific non-portable module is rebuilt. The npm package phantomjs-prebuilt is ignored because the module does not contain any files ending in ".node" and thus meteor thinks it is portable. I think looking for ".node" files is a heuristic for packages that use node-gyp, but it fails to flag phantomjs-prebuilt because it uses a non-portable install script to pull down arch-specific binaries. Full logic is in the function isPortable: https://github.com/meteor/meteor/blob/devel/tools/isobuild/meteor-npm.js#L441

I confirmed that changing the code to see phantomjs-prebuilt as non-portable caused the correct rebuild to happen and fix the reported issue.

I think this could be fixed by additionally look in each package to see if the scripts section of package.json contains a install or preinstall script. The npm docs say "[t]he only valid use of install or preinstall scripts is for compilation which must be done on the target architecture." This will add some file read and parsing time in additional to causing a few more rebuilds to happen but it should give correct behavior. Additionally I suspect instead of heuristically looking for .node files a check could directly be made for a .gyp config file, but that may not be true in all cases and would at least break the meteor-force-non-portable package.

Note: I noticed while debugging the dispatch:phantomjs-tests package was actually packaged with cached portable status files like ~/.meteor/packages/dispatch_phantomjs-tests/0.0.7/npm/node_modules/phantomjs-prebuilt/.meteor-portable which contained the value true. I'm not sure if that is intended behavior, but it does suggest two things:

  1. Any fix will require publishing updated versions of dispatch:phantomjs-tests and other affected packages with fixed or removed .meteor-portable files
  2. An ugly short-term workaround: simply re-publish the package with the cache file for phantomjs-prebuilt manually edited to contain false. That should force a rebuild on install, the cached value will override any portability logic.
@aldeed

This comment has been minimized.

Contributor

aldeed commented Jan 25, 2017

Wow, that seems promising @danstiner. Thanks for investigating! I will see if I can get a fixed version published using the workaround.

@abernix

@benjamn

This comment has been minimized.

Member

benjamn commented Feb 4, 2017

@danstiner That analysis checks out with me (and I wrote the logic that you're analyzing)!

I think this could be fixed by additionally look in each package to see if the scripts section of package.json contains a install or preinstall script.

I think this is a totally reasonable approach. I would be cautiously open to going back to the old behavior of rebuilding everything, but this middle ground strikes me as a safe compromise. If it's safe to rebuild everything, then it's safe to rebuild a few additional packages.

Would you have time to submit a pull request for that change? Specifically, make isPortable return false if a package.json file containing an install or preinstall script is found?

benjamn added a commit that referenced this issue Feb 9, 2017

Consider npm packages with {,pre,post}install scripts non-portable.
Inspired by analysis from @danstiner:
#8225 (comment)

Fixes #8225, as well as the tests I added in 672c4f3.

I changed the name of the .meteor-portable file to .meteor-portable-1.json
in order to invalidate previous .meteor-portable files. This naming scheme
will be more sustainable, because we can keep incrementing the version
number whenever we change this logic.

@benjamn benjamn added this to the Release 1.4.3 milestone Feb 9, 2017

@benjamn benjamn added the known-fix label Feb 9, 2017

@danstiner

This comment has been minimized.

danstiner commented Feb 10, 2017

Thanks so much for taking care of this @benjamn! Sorry I wasn't able to make time to send a pull request myself.

@lpgeiger

This comment has been minimized.

lpgeiger commented Feb 10, 2017

@benjamn I can confirm that the meteor update --release 1.4.3-rc.3 runs dispatch:mocha-phantomjs smoothly, and fixes issues in meteor/todos#198 , DispatchMe/meteor-mocha-phantomjs#33 and possibly DispatchMe/meteor-mocha-phantomjs#35

@abernix If MDG is updating the testing guide: could you focus more on dispatch:mocha-phantomjs, rather than on the other dispatch options ? Despite this error it seems like dispatch:mocha-phantomjs seems to be currently the most maintained package of them all.

@aldeed

This comment has been minimized.

Contributor

aldeed commented Feb 10, 2017

@keyscores All the dispatch: testing packages are fully maintained (mostly by me) and have no serious issues as far as I know other than this binary rebuild issue.

@lpgeiger

This comment has been minimized.

lpgeiger commented Feb 10, 2017

@aldeed Sorry I meant in regards to practicalmeteor:mocha
While I have you here, is there a reason there are different dispatch packages? Is there an opportunity to consolidate the features in the phantomjs package?

@benjamn

This comment has been minimized.

Member

benjamn commented Feb 10, 2017

By the way, @aldeed, although this issue was in the back of my head, I independently encountered the same problem while trying to add the tools/tests/apps/modules app to the meteor self-test suite the other day (worked fine locally, failed on Linux CI servers): 672c4f3

I'm really happy to get this fixed, because running the modules tests automatically is something I've been meaning to do ever since Meteor 1.3, and dispatch:mocha-phantomjs is now working perfectly for that task. Thank you for maintaining it!

@lpgeiger

This comment has been minimized.

lpgeiger commented Feb 10, 2017

@benjamn thanks for pushing the fix. Can you confirm that your test rig is using meteor-mocha-phantomjs@=0.1.9 . The patch fixed issues at 0.1.7 installed in meteor/todo. But I'm getting a regression when upgrading meteor-mocha-phantomjs to 0.1.9.

@aldeed

This comment has been minimized.

Contributor

aldeed commented Feb 10, 2017

@keyscores mocha-phantomjs should stay separate since it has the phantomjs dependency that complicates things. In theory dispatch:mocha and dispatch:browser could be combined and would still be very lightweight. The client code would just never run if you had a server-only app. That wouldn't be the worst idea.

As far as I remember, the one thing some people like about the practicalmeteor package is that it shows the server tests in the browser alongside client tests rather than in the server terminal. But in order to do that, it is bloated with a bunch of extra code, which to me is not worth it.

@benjamn Nice! and you're welcome.

@lpgeiger

This comment has been minimized.

lpgeiger commented Feb 10, 2017

@aldeed I think it would be great to consolidate a bit. I'm for any solution that allows me to run client and server tests locally and in CI. Phantomjs seems to be the way to do this. Couldn't you consolidate in a way that checks is phantomjs is installed, and if not fall back on other methods?

@benjamn benjamn closed this in bcffe53 Feb 11, 2017

@aldeed

This comment has been minimized.

Contributor

aldeed commented Feb 11, 2017

@keyscores I don't see any benefit to consolidating further. You can add multiple driver packages and multiple npm scripts for them, and then run any script you want on CI and any script you want locally. And if you never want to run client tests in your browser, then you only need the phantomjs one anyway. Complicating the packages will just make them harder for others to understand and contribute to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment