You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by Mark Koudritsky <ka...@gmail.com> on 2013/12/07 00:50:31 UTC
Review Request 16099: Pass cli args to hooks, fix hooker.spec on windows.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16099/
-----------------------------------------------------------
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
Re: Review Request 16099: Pass cli args to hooks, fix hooker.spec on windows.
Posted by Michal Mocny <mm...@chromium.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 16099: Pass cli args to hooks, fix hooker.spec on windows.
Posted by Mark Koudritsky <ka...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16099/#review30038
-----------------------------------------------------------
e2e/hooker.spec.js
<https://reviews.apache.org/r/16099/#comment57545>
For non-win platforms there is logic to skip bat files (with a warning). But looks like on windows .sh files will be executed.
The old version was just copying the right files from the fixtures dir as part of the test. Moving the extra complexity out from the code into the folder structure seemed more elegant to me (though that's entirely a matter of taste).
e2e/hooker.spec.js
<https://reviews.apache.org/r/16099/#comment57546>
The hooker does have logic to execute the handlers serially, wrapping them in Q promises. It's here: https://github.com/apache/cordova-cli/blob/master/src/hooker.js#L142
But this test wasn't really checking it, I've changed h1 to return a promise that resolves after a delay of 0.1 second.
e2e/hooker.spec.js
<https://reviews.apache.org/r/16099/#comment57547>
That's right, but this is an error handler passed to Q.then, I don't think it can ever be called with no error argument.
But I changed this to a different pattern that looks like this:
fire(test_handler)
.then(expectations)
.fail(err_handler)
.fin(done)
This one is better if an exception is raised by some code in the expectations checking function. In the previous case it's that exception would be silented by the Q lib + jasmine because Q has no error handler to pass it to and jasmine ignores the re-raised exception as long as done() is called by the .fin().
e2e/hooker.spec.js
<https://reviews.apache.org/r/16099/#comment57548>
The credit is not mine, was there before :)
e2e/hooker.spec.js
<https://reviews.apache.org/r/16099/#comment57549>
The credit is not mine, was there before :)
- Mark Koudritsky
On Dec. 9, 2013, 10:19 p.m., Mark Koudritsky wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16099/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2013, 10:19 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_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
>
>
Re: Review Request 16099: Pass cli args to hooks, fix hooker.spec on windows.
Posted by Michal Mocny <mm...@chromium.org>.
Alright, pushed. You can mark the issues resolved.
On Tue, Dec 10, 2013 at 11:19 AM, Michal Mocny <mm...@chromium.org> wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16099/
>
> Ship it!
>
> Looks good. Want to email me a format-patch so I have author information?
>
>
> - Michal Mocny
>
> On December 9th, 2013, 10:38 p.m. UTC, Mark Koudritsky wrote:
> Review request for cordova.
> By Mark Koudritsky.
>
> *Updated Dec. 9, 2013, 10:38 p.m.*
> *Bugs: * CB-4382 <https://issues.apache.org/jira/browse/CB-4382>, CB-5330<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/hv
>
>
>
> - 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.
>
> Testing
>
> npm test
> cordova prepare with dummy prepare hooks that store all environment variables.
>
> Diffs
>
> - e2e/create.spec.js (3f1304c)
> - 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)
>
> View Diff <https://reviews.apache.org/r/16099/diff/>
>
Re: Review Request 16099: Pass cli args to hooks, fix hooker.spec on windows.
Posted by Michal Mocny <mm...@chromium.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16099/#review30102
-----------------------------------------------------------
Ship it!
Looks good. Want to email me a format-patch so I have author information?
- Michal Mocny
On Dec. 9, 2013, 10:38 p.m., Mark Koudritsky wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16099/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2013, 10:38 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/hv
>
> - 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_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
>
>
Re: Review Request 16099: Pass cli args to hooks, fix hooker.spec on windows.
Posted by Mark Koudritsky <ka...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16099/
-----------------------------------------------------------
(Updated Dec. 9, 2013, 10:38 p.m.)
Review request for cordova.
Changes
-------
add a forgotten "var"
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 (updated)
-------
Note: Two commits in diff (created as git format-patch). Same diff on github: https://github.com/kamrik/cordova-cli/compare/hv
- 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 (updated)
-----
e2e/create.spec.js 3f1304c
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
Re: Review Request 16099: Pass cli args to hooks, fix hooker.spec on windows.
Posted by Mark Koudritsky <ka...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16099/
-----------------------------------------------------------
(Updated Dec. 9, 2013, 10:19 p.m.)
Review request for cordova.
Changes
-------
- removed the e2e/fixtures/hooks dir, it was left there by mistake, all hooks are in hooks_sh and hooks_bat.
- addressed the comments from previous revision.
Same diff on github: https://github.com/kamrik/cordova-cli/compare/hv
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 (updated)
-----
e2e/create.spec.js 3f1304c
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