cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michal Mocny" <mmo...@chromium.org>
Subject Re: Review Request 16099: Pass cli args to hooks, fix hooker.spec on windows.
Date Mon, 09 Dec 2013 19:20:46 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message