----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16099/#review30029 -----------------------------------------------------------
e2e/hooker.spec.js <https://reviews.apache.org/r/16099/#comment57531> Would it be wrong to just have the .sh and .bat live in the same input directory and copy all the files on all platforms? e2e/hooker.spec.js <https://reviews.apache.org/r/16099/#comment57532> Is return Q() necessary? Not sure what it does here. [edit].. hmm I see in a later test that maybe this marks the handler as being async? Anyway, seems we have a duplicate test here anyway? I'll come poke you to discuss. e2e/hooker.spec.js <https://reviews.apache.org/r/16099/#comment57533> If error callback is called with no argument, that should be a failed test, but right now would pass, right? e2e/hooker.spec.js <https://reviews.apache.org/r/16099/#comment57534> I approve of this test data. src/hooker.js <https://reviews.apache.org/r/16099/#comment57535> Personally I like to see whitespace inside the ternary operator src/hooker.js <https://reviews.apache.org/r/16099/#comment57536> Do we have sample hooks scripts that try to parse these environment variables? Seems right now these are comma separated without quotations, which seems fine, but not sure what the standard is for shell environment variable lists on windows etc.. - Michal Mocny On Dec. 6, 2013, 11:50 p.m., Mark Koudritsky wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16099/ > ----------------------------------------------------------- > > (Updated Dec. 6, 2013, 11:50 p.m.) > > > Review request for cordova. > > > Bugs: CB-4382 and CB-5330 > https://issues.apache.org/jira/browse/CB-4382 > https://issues.apache.org/jira/browse/CB-5330 > > > Repository: cordova-cli > > > Description > ------- > > Note: Two commits in diff (created as git format-patch). Same diff on github: > https://github.com/kamrik/cordova-cli/compare/hook_vars > > - Refactored the hooker.spec.js to use real files like the e2e tests. > - Moved the spec and corresponding fixtures to live under e2e dir. > - Rearranged the hooks fixtures into two separate dirs for Win and non-Win > platforms. > - e2e tests on windows can interfere with one another (seem to run partially > in parallel), changed them to use different tmp subdirs for each test. > > > Diffs > ----- > > e2e/create.spec.js 3f1304c > e2e/fixtures/hooks/fail/fail.bat PRE-CREATION > e2e/fixtures/hooks/fail/fail.sh PRE-CREATION > e2e/fixtures/hooks/test/07.bat PRE-CREATION > e2e/fixtures/hooks/test/07.sh PRE-CREATION > e2e/fixtures/hooks/test/1.bat PRE-CREATION > e2e/fixtures/hooks/test/1.sh PRE-CREATION > e2e/fixtures/hooks_bat/fail/fail.bat PRE-CREATION > e2e/fixtures/hooks_bat/test/.dotted.bat PRE-CREATION > e2e/fixtures/hooks_bat/test/07.bat PRE-CREATION > e2e/fixtures/hooks_bat/test/1.bat PRE-CREATION > e2e/fixtures/hooks_sh/fail/fail.sh PRE-CREATION > e2e/fixtures/hooks_sh/test/.dotted.sh PRE-CREATION > e2e/fixtures/hooks_sh/test/07.sh PRE-CREATION > e2e/fixtures/hooks_sh/test/1.sh PRE-CREATION > e2e/helpers.js aa1c790 > e2e/hooker.spec.js PRE-CREATION > e2e/platform.spec.js be5761e > e2e/plugin.spec.js dd493bb > package.json 6c1c753 > spec/fixtures/hooks/fail/fail.bat 0c810b7 > spec/fixtures/hooks/fail/fail.sh 379a4c9 > spec/fixtures/hooks/test/07.bat 1095fc0 > spec/fixtures/hooks/test/07.sh 6e25461 > spec/fixtures/hooks/test/1.bat 4e76af0 > spec/fixtures/hooks/test/1.sh 53d5e97 > spec/hooker.spec.js d4b073e > src/hooker.js 06acec7 > > Diff: https://reviews.apache.org/r/16099/diff/ > > > Testing > ------- > > npm test > cordova prepare with dummy prepare hooks that store all environment variables. > > > Thanks, > > Mark Koudritsky > >
