cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From agrieve <...@git.apache.org>
Subject [GitHub] cordova-lib pull request: CB-8239 Add support for git urls to 'cor...
Date Sun, 18 Jan 2015 01:59:39 GMT
Github user agrieve commented on a diff in the pull request:

    https://github.com/apache/cordova-lib/pull/148#discussion_r23130172
  
    --- Diff: cordova-lib/spec-cordova/util.spec.js ---
    @@ -183,4 +184,138 @@ describe('util module', function() {
                 expect(res.indexOf('CVS')).toEqual(-1);
             });
         });
    +    describe('getPlatformDetailsFromDir function', function () {
    +
    +        var dir = 'C:\\Projects\\cordova-projects\\cordova-android';
    --- End diff --
    
    I hate to be too harsh, but I really don't think these tests add any value (suspect they
are just maintenance burden).
    
    The signs to me are:
    1. There is more code here to mock out things than there is to actually test.
    2. Windows-style paths, and they work perfectly fine on OS X. They really shouldn't but
you're mocking out so much that nothing is really being exercised in a way that resembles
reality.
    3. Mocking out of private functions 
    
    In an ideal world, tests will still work when implementations change so long as APIs remain
the same. I this is too far from the truth, then the presence of the tests will hurt the project
more than help it, because it will hurt the ability to make changes to the code.
    
    So.. I'd be fine with this landing without new tests, but I don't think I'd land it with
these ones.
    
    If you'd like to have another stab at it, I think the biggest problem is that the code
itself is difficult to test. This usually means a refactor will improve both the testability
and quality of the code (generally, testable code is well-factored code). Likely introducing
dependency injection will help as well (e.g. create a "Downloader" class). We can't change
the platform.js API, but it's perfectly fine to create a new API, test that, and then write
some glue to make the old API call the new one.
    
    If you're not up for refactoring / rewriting the tests, totally understandable. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org


Mime
View raw message