cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mark Koudritsky" <kam...@gmail.com>
Subject Re: Review Request 16099: Pass cli args to hooks, fix hooker.spec on windows.
Date Mon, 09 Dec 2013 22:20:44 GMT

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


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