readium / readium-js-viewer

👁 ReadiumJS viewer: default web app for Readium.js library
BSD 3-Clause "New" or "Revised" License
553 stars 186 forks source link

package.json hack to fix #566: add symlink step to prepare script #567

Closed da70 closed 8 years ago

da70 commented 8 years ago

Hack to fix automated Selenium tests in current develop and master branches.

This pull request is Finalized

Related issue(s) and/or pull request(s)

See #566.

danielweck commented 8 years ago

thank you! first of all, package.json is auto-generated from CSON master files (see the 'package' folder). so your changes will be overridden. secondly, the 'ln' symbolic linking command is not cross-platform. we are working hard to keep the build process compatible with Windows DOS, by creating NodeJS scripts or by using existing NodeJS libs that implement filesystem manipulations, etc. I am closing this PR, but the issue you filed is definitely worth looking into. I personally have had no problems running the tests, but I will check again. PS: try running he CFI unit tests to see if this works at your end, please (Firefox and Chrome)

da70 commented 8 years ago

Ah yes I forgot about the CSON master files, and good point about the Windows compatibility.

I have updated the bug report with more detailed instructions. It is likely that the tests are not breaking for you because your clone was created and initialized before Feb 18, 2016. That was the day that phantomjs was removed as a dependency. If you delete that module and try running your tests again you will likely get the bug. Or, you could make a completely new clone and run the tests (that's how I discovered the problem), or else delete node_modules/ completely and re-build it.

I will try the CFI unit tests. Thanks!

danielweck commented 8 years ago

awesome feedback, thanks!

da70 commented 8 years ago

Sure. Also, I ran the CFI tests. At first they failed due to missing peer dependency jasmine-core. I checked the latest Travis build job for develop -- Job # 137.1 -- and noted that it used older versions of Node and NPM (v0.12.15 and v2.15.1, respectively). That's when I remembered that we can't use a Node version > 4.x due to #453. I've been using 6.0.0 with its higher NPM version.

I installed Node 0.12.15 and was able to initialize a new clone of readium-cfi-js and run all tests with 0 failures. I then switched to Node 4.1.1 and was able to replicate the steps and full test suite pass.

When I switched to master branch though, I could not complete npm run build, even using older Node versions. Error:

demo$ npm run build

> readium-cfi-js@0.24.0 prebuild /private/tmp/readium-cfi-js
> npm run clean

> readium-cfi-js@0.24.0 clean /private/tmp/readium-cfi-js
> node --version && npm --version && node ./node_modules/rimraf/bin.js build-output/* && node ./node_modules/rimraf/bin.js api-docs/* && node ./node_modules/rimraf/bin.js gen/*

v0.12.15
2.15.1

> readium-cfi-js@0.24.0 build /private/tmp/readium-cfi-js
> npm run build:scripts

> readium-cfi-js@0.24.0 prebuild:scripts /private/tmp/readium-cfi-js
> npm run build:scripts:peg

> readium-cfi-js@0.24.0 build:scripts:peg /private/tmp/readium-cfi-js
> node ./node_modules/pegjs/bin/pegjs --export-var window.EPUBcfiParser cfi_parser.pegjs gen/cfi_parser_gen.js

Can't use the -e/--export-var option with the "commonjs" module format.

npm ERR! Darwin 14.5.0

[...SNIPPED...]

I didn't see any Travis jobs for master, so didn't go any farther with it.

To be thorough, I tested #566 again using Node 4.1.1, and the error persisted (which makes sense since it's not a problem related to peer dependencies).

danielweck commented 8 years ago

ah, we need to cherry pick a develop fix to master! thanks for pointing this out (and for all your tests)

da70 commented 8 years ago

Sure, glad I could help.