I went over the PasscodeHelper with ttaubert, who suggested a few things which I have been more aware of (and now realize, Greg even suggested doing earlier - I apologize!) Here's the gist: Almost all state is useless and could be handed around from function to function, which avoids existing race condition problems. This also makes the whole function constructor pattern useless, so I'll move away from it. Also, there are some indentation errors which I am going to fix along the way.

I hope this resolves the existing problems with statefulness and race conditions. Looking forward to seeing your feedback!

You now put all helping functions in the huge closure of passcode_helper.js, which is invisible even for tests, thus the unit test couldn't test them. The similar style has caused lots of troubles in v1.3 window manager. So I suggest you still keep a literal object style that people could still access these helping methods when it's necessary (for example, we may need to write more unit tests for it). That is, rather than: (function(exports) { function _makeDigest() { /**/ }; function setPasscode() { /**/ }; //.... exports.PasscodeHelper = { /**/ }; )(window); It would be better to have: (function(exports) { var PasscodeHelper = { _makeDigest() { /**/ }, setPasscode() { /**/ }, // ... }; exports.PasscodeHelper = PasscodeHelper; )(window); In my recollection I've noticed you that this literal object pattern is OK if you have no states. And the style you used reminds that your current unit test isn't so *unit* actually. From the methods you exposed and tested, the test is actual some "headless integration test", or API test. Since for unit tests we should write tests for each method. And it isn't worth to write code in the closure style just to distinguish the "private" and public methods as what other OO languages do. In JavaScript you just don't have well supports for "private" data & functions, and do to with closure and other tricks would easily mess the code. So if you have no other concerns I suggest you could just write the code in the literal object style, and avoid to use closure.

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #2) > You now put all helping functions in the huge closure of passcode_helper.js, > which is invisible even for tests, thus the unit test couldn't test them. I wold prefer not to expose the other methods, so they will not be used independently. It's a conscious decision to expose only set() and check(), so I can change implementation and improve security without the concern of some app using other methods. The test for check() and set() exercise all internal functions and are thus tested in a high-level way. I would really prefer not to expose the other methods. > The similar style has caused lots of troubles in v1.3 window manager. So I > suggest you still keep a literal object style that people could still access > these helping methods when it's necessary (for example, we may need to write > more unit tests for it). That is, rather than: > > (function(exports) { > function _makeDigest() { /**/ }; > function setPasscode() { /**/ }; > //.... > exports.PasscodeHelper = { /**/ }; > )(window); > > It would be better to have: > > (function(exports) { > var PasscodeHelper = { > _makeDigest() { /**/ }, > setPasscode() { /**/ }, > // ... > }; > exports.PasscodeHelper = PasscodeHelper; > )(window); > > In my recollection I've noticed you that this literal object pattern is OK > if you have no states. And the style you used reminds that your current unit > test isn't so *unit* actually. From the methods you exposed and tested, the > test is actual some "headless integration test", or API test. Since for unit > tests we should write tests for each method. > Yes, the helper is now stateless. > And it isn't worth to write code in the closure style just to distinguish > the "private" and public methods as what other OO languages do. In > JavaScript you just don't have well supports for "private" data & functions, > and do to with closure and other tricks would easily mess the code. So if > you have no other concerns I suggest you could just write the code in the > literal object style, and avoid to use closure. Sorry, but I do not completely understand this concern.

(In reply to Frederik Braun [:freddyb] from comment #3) > (In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #2) > > You now put all helping functions in the huge closure of passcode_helper.js, > > which is invisible even for tests, thus the unit test couldn't test them. > > I wold prefer not to expose the other methods, so they will not be used > independently. It's a conscious decision to expose only set() and check(), > so I can change implementation and improve security without the concern of > some app using other methods. FWIW, this in line with how we write our modules for Fx Desktop as well. The API should only expose what is needed, having to refactor tests that know too much about the implementation when refactoring a large chunk of code is a pain. If tests are well written to exercise all of the functionality I don't see a reason either to expose private functionality. > > In my recollection I've noticed you that this literal object pattern is OK > > if you have no states. And the style you used reminds that your current unit > > test isn't so *unit* actually. From the methods you exposed and tested, the > > test is actual some "headless integration test", or API test. Since for unit > > tests we should write tests for each method. This is certainly not how I remember unit testing. Unit testing is (IMO) explicitly about testing functionality exposed by the API, and yes, testing every method that the API offers. That explicitly excludes private functions that are a detail of implementation. > > And it isn't worth to write code in the closure style just to distinguish > > the "private" and public methods as what other OO languages do. In > > JavaScript you just don't have well supports for "private" data & functions, > > and do to with closure and other tricks would easily mess the code. So if > > you have no other concerns I suggest you could just write the code in the > > literal object style, and avoid to use closure. JS does support "private" data and functions. Closures have been the way to go for years. Closures are not "tricks", they are a basic language feature that make JS as great as it is. I understand that there might be different styles applied to Fx OS and Desktop but we should acknowledge and use common and popular patterns. If you look at Firefox Desktop modules (.jsm files) then you can tell that this is exactly how we implement APIs without exposing private functionality. Take a look at tons of Node.js modules that do it all the same way. This pattern seems to work pretty well for a huge community of people.

Gregn, can you please advise how to proceed?

I can only suggest you to prevent failures we'd experienced. I *do* know closure is totally legal to implement "private" in JS, while what I concern is whether test could had enough coverage to make sure wrong things could ve corrected as easy as possible. Also, I *do* know there even are some querrials in C++ world about whether private methods should be tested in unit tests, so I don't want to argue that again. What I consider is a simple thing: the more you test the less it would fail, especially for a language without compiler and type supports. Think about which way is easier to pick up your possible errors in your implementation: to test only the final exposed two methods and trace back from the generated result step by step, or to test each methods at unit test so we could guarantee that each step is correct already? At least for me, to put this with the possibilities user may use these inner function incorrectly (I would blame the lack of docs and the user first), I would chose to care not to reduce the coverage to enhance the encapsulation in this way. You also argued about tons of people expose only public functiona without problem, but I could tell you at least with the code my colleagues and me had involved, we finally found it's too painful to work with such closure pattern, especially when the code grows up in a rate you never thought at beginning. But maybe for you and your patch, it's hard to grow the code up for this specific domain problem, so you're just fine to adopt the pattern. However, it doesn't means the pattern is perfact pattern, at least not accroding to your "people love that, so there is no problem", since I could immediately find a counterexample that we couldn't love it after so many troubles, and it must depends on the domain to decide whether it's true, or just partially true. So please don't think that I don't have any sence about such issue, I just feel it's unworthy to do the trade-off, especially when to closured things is not truly equal to make things "private", because closure is more about literal scoping issue than classes scoping. Anyway, since I'm not entitled peer or owner, you don't need my grant to land it. What I typed is just want to fully explain my concerns to prevent there are any misunderstings between us. So it's totally okay that you find another regular peer or owner to land your code (and since autolander requires that, it becomes necessary).

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #6) > … What I typed is just want to fully explain my concerns to prevent > there are any misunderstings between us. So it's totally okay that you find > another regular peer or owner to land your code (and since autolander > requires that, it becomes necessary). Yes, and I *really* appreciate your comments. I don't want to degrade Gaia code quality willfully. But I also don't want to do needless extra work either ;-) Thank you Greg! :) Kevin, what do you suggest? Should I write tests for all functions and expose everything, or should I just write unit tests for the exposed API? See also comment 2 and the discussion following it.

FWIW, as long as we can resist the temptation to introduce states into PasscodeHelper (inc. runtime switches) nor adding too many helper functions to it, FOREVER, I think the patch is fine. The problem with JavaScript in the browser is that you don't get a closure/context for free per file, nor you could synchronously get another module/class/function ... that's why we ended up settle with constructor pattern, in which unnecessary exposure of detail is favorable over accidentally, untestable, big closures. But again, the patch is good and we should block on academic discussions.

... we should *not* block on academic discussions.

It seems like we've failed to make a distinction of best practices here, and that's coming back to bite us. We should probably sit down and decide on a single pattern to decide on for shared libraries. Either export everything as an instance of a class, an entire object, or just what's needed. I have a long term vision of how I'd like us to craft these libraries, and it involves harmony classes, which should be clean and make everything testable. I haven't yet socialized this idea much though, but will start doing so as it seems we have a need for it. I would just recommend that people be flexible here, and agree with Tim that we shouldn't block this from landing for now. If there's some reason we need additional test coverage it seems like it's easy enough to expose additional methods which I'm fine with. Assuming we have decent test coverage, and are ok with exposing new methods to test when necessary, I think I am fine leaving an R+ here until we decide on a gaia-wide best practice. Once we decide on that, it's likely we'll need to revisit this as we refactor most of our shared libraries.

Comment on attachment 8556412 [details] Github Pull Request Thanks for the detailed replies! That really does answer my question. I'll rebase and squash once this gets an r+.

Comment on attachment 8556412 [details] Github Pull Request It seems Tim has done a solid review, I took a quick look and this looks pretty good to me. I'll leave an R+ here so you can get this landed, but I do think we'll need to revisit this in the future (as well as all of the shared libraries), once we define our desired module pattern.

Green try run: https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=a35397629fa6 Requesting checkin-needed when Gaia is green again

rebased with master and re-wording the commit message to make autolander happy.

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/ab7431231c2095c0026e37ee0f5473359c797213