Unit test scoping problems, thoughts, potential solution

28 views
Skip to first unread message

Frrz Novles

unread,
Dec 31, 2017, 4:40:19 PM12/31/17
to greasemo...@googlegroups.com
Some portions of the Greasemonkey code base are wrapped in immediately invoked functions (IIFs) in order to privately scope function definitions and variables. For example the script registry[1] defines many variables that are scoped only to the IIF. For the rest of this we'll stick with the registry as our example.

The goal of unit tests is to control as much external state as possible so that we're only testing singular items. Unfortunately, with the scoping problems this isn't the case in the current registry tests. In order to check if a script exists all tests are relying on an external function `scriptsToRunAt`[2]. Ideally the test would just check the object directly. Further, both tests in the file are dependent on `_saveUserScript`. Again, it would be ideal if the script was injected into the `userScripts` object (database could be mocked or preloaded with good data, or moved to a separate function that can be `sinon.stub` and therefore assumed good for the purpose of tests not related to that function).

One solution is to do the same thing that occured with `_saveUserScript` and `_loadUserScripts`, and that's export the object. And by extension anything that is required for  testing would be exported (anywhere in the file). If it's not needed for testing then it, rightly, doesn't need to be exported. Since Greasemonkey is a closed system I feel that doing the above is a valid solution. Closed system meaning, Greasemonkey isn't a 'library' wherein some external provider that calls into it and directly accesses or creates Greasemonkey objects, unlike an AngularJS module or some-such.

Alternatively, exposing these references could be done conditionally, only inside the development environment[3]. This method is more complex than the previous one. And it would rely on modifying the `package.sh` script to account for the conditionals, or move to using something like grunt.

Lastly, I suppose you could use the class construct. This is similar to the initial option, but somewhat tangent. Rather than picking and choosing what to export on a simple JS object, the idea is to just create a new class and export the instance of it. Due to the 'publicness' of JS, this also solves the problem. Although, care needs to be taken if you want to spy on a constructor when using  the ECMS6 syntax (due to where the class reference is scoped [not on `window`] it's a bit weird). And of course this may not work with all portions of the code. It'll probably work nicely with the registry but it may not in other sections.

Thoughts? (That seems quite long..).

- Steven

Anthony Lieuallen

unread,
Dec 31, 2017, 6:57:25 PM12/31/17
to greasemo...@googlegroups.com
On Sun, Dec 31, 2017 at 4:40 PM, Frrz Novles <googo...@gmail.com> wrote:
The goal of unit tests is to control as much external state as possible so that we're only testing singular items. ...

Testing is a weird subject.  I don't like most of the words people tend to use (like "unit") in this context, because I find that different individuals often have critically different interpretations of the meaning of these words, so discussions become difficult quickly.

That said, I disagree.  My general philosophy is to test behaviors more than "units".  I have a (strong!) preference for tests that have the property wherein you can change the code (but not the usually-high-level behavior's interface) and they still pass.  Targeting units as small as a single function usually means you have to change your tests too often/write more tests, even when the intended external behaviors have not changed.  So for example, I like tests of the form "When the user presses Ctrl+S the script's next execution will include the new code in the editor."  (Because e.g. it got saved to IndexedDB, and that's where it gets loaded from for execution -- but not explicitly!  If that changes in the future, the test is still 100% valid!)  And not of the form "When __EditorSave() is called, then UserScript.save() is called".  (Which could break for all kinds of valid refactoring reasons which don't affect the end user visible behavior.))

(Good coverage is one of the exceptions I tend to end up finding myself in.  Edge cases can be difficult to test for/set up, so sometimes I target a really tiny unit, just to cover an edge case.)
 
One solution is to do the same thing that occured with `_saveUserScript` and `_loadUserScripts`, and that's export the object. ...

First: lots of the existing code, and also low quality of the tests, is thanks to rushing to become at least minimally functional before Firefox 57.  That said, I'd generally prefer to test a high level behavior (and not a private implementation detail!), and not cheat by just making the private implementation detail public.  (If this were Java I'd say "don't use @VisibleForTesting" https://developer.android.com/reference/android/support/annotation/VisibleForTesting.html .)

Frrz Novles

unread,
Dec 31, 2017, 8:47:19 PM12/31/17
to greasemo...@googlegroups.com
On Sun, Dec 31, 2017 at 5:57 PM, Anthony Lieuallen <aran...@gmail.com> wrote:

That said, I disagree.  My general philosophy is to test behaviors more than "units".  I have a (strong!) preference for tests that have the property wherein you can change the code (but not the usually-high-level behavior's interface) and they still pass.


Sure, it's good to ensure that end user behavior is working as intended, but I feel that tests that only focus on that are stuck in a position of being too broad. With your above test case, let's say the code is not in the editor (therefore the test failed). You're now stuck debugging like 6-7 functions to figure out where the data transfer failed. I feel like this is entirely counter to what testing should provide, that is less time trawling entire code paths to figure out where something stuck.

Would it not be beneficial to break the above into three or four individual tests along the lines of:
- Does the editor send the correct data when Ctrl+S is sent?
- When the data is received is the correct data stored in the database?
- Is the modified data sent back to the editor?
- Does the editor handle the response correctly?

In essence your one test is doing all of the above. When it fails, figuring out which particular event is causing the problem becomes more difficult, no? The middle two are 'implementation' and you're saying that they shouldn't be tested directly? Am I missing something?

Reply all
Reply to author
Forward
0 new messages