[
https://issues.apache.org/jira/browse/CB-4036?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13720815#comment-13720815
]
Andrew Grieve commented on CB-4036:
-----------------------------------
Hey Tim,
Great to see the platform checks moving forward! Probably should leave the
<engine> tag in though?
Had a look at the unit test you just added, and the number of mocks raises an
eyebrow I think. Too many mocks falls into the code smells category I think
("overspecified mocks"), and can lead to tests that fail when implementations
are refactored (e.g., if it used chmod instead of chmodSync, the test would
fail). It's not actually useful to assert that the code calls chmodSync.
Another thing to point out is that mocking out functions on the action stack is
definitely testing implementation rather than specification.
NPM doesn't seem to have any complete fs replacements
(https://npmjs.org/package/fs-in-memory seems right, but is incomplete).
Probably wouldn't be too hard to add missing functions to it.
For this particular function, all you're trying to test is that the code that
checks platform versions correctly. I think a more direct way to do this would
be to extract this logic into a helper function that takes all of its inputs as
parameters. Then, you can test the helper method directly.
I don't mean too much to tell you how you should write tests, and I realize you
probably are following a pattern of testing that already exists. But, I've been
on a project before that had over-specified tests, and it really killed our
productivity (and morale). Every innocent code change would cause tests to
break, and we'd end up spending more time fixing tests than we would fixing the
code. Worst part was that the tests often failed to catch bugs because the
mocks made them not line up with reality.
> <platform> tag should have a "version" attribute
> -------------------------------------------------
>
> Key: CB-4036
> URL: https://issues.apache.org/jira/browse/CB-4036
> Project: Apache Cordova
> Issue Type: Improvement
> Components: Plugman
> Reporter: Shazron Abdullah
> Assignee: Filip Maj
>
> This might be hole that we didn't consider.
> I know implicitly if the plugin supported an "engine" version we support what
> the engine supports, but here could be one scenario.
> For example, with iOS 7, a plugin CDVFooBar use this awesome NSWhizBang
> framework. Fine, with iOS 7, you have to of course build with the iOS 7 SDK,
> and you can support iOS 6 with a Deploy Target build setting.
> It runs on iOS 7 - yay.
> It.. blows up on iOS 6 at runtime, because of course NSWhizBang framework
> does not exist on iOS 6.
> Now you say, why don't you do Obj-C runtime checks and weak link the
> framework? Yes, the plugin author can do that but a plugin user, by parsing
> the plugin xml (using a tool, or optically, whatever) cannot know that the
> plugin does NOT work on iOS 6 - and even though it "runs" on iOS 6, it does
> nothing, which is useless and wastes a lot of time.
> So - my proposal is, to add a *"version"* attribute on the *<platform>* tag.
> It should follow the syntax for the <engine> version attribute.
> What would be the default if the version attribute is not there? Not sure
> what a reasonable default is yet.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
