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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org
