GNOME Bugzilla – Bug 719918
[RFC] Encapsulate tests and get them running in a normal test runner
Last modified: 2017-08-19 07:59:40 UTC
Currently, it is very difficult to run the unit tests. They require twiddling of environment variables in order to search the right paths and most of them bring in UI code and other dependencies. It would be desirable to have the tests running under a unified test runner, such that running the test runner can run all tests, ideally no matter which language they are written in. This patch series creates a GTest based test-runner for the javascript tests and then also refactors them such that each individual test is within its own fixture or function. It also does some refactoring on the main codebase to ensure that the js files we depend on in the tests don't bring in UI code that is much harder to test (for now). Run the test runner like this: ./src/discover-js-unit-tests -j tests/unit -i js/ -f src/ -j: Path to a directory containing test files. -i: Include directory to append to javascript search path. -f: Directory to append to typelib search path. This is currently only in RFC state at the moment. There's a few memory leaks (which I need to hunt down) and also some tests which rely on extending the String prototype which doesn't seem to work the way I expect it to. Also, it seems like adding options to a gtest test binary isn't such a good idea and we might have to switch to using environment variables there as the binary is not gtester friendly at the moment.
Created attachment 263603 [details] [review] Pull fixMarkup out into its own file
Created attachment 263604 [details] [review] Reimplement notifyError in util.js
Created attachment 263606 [details] [review] Added a framework for providing introspectable test fixtures for GTest. We want to write tests in JavaScript, but there's no real test runner story yet. This commit provides some integration glue between our javascript tests and GTest, the framework used to run other tests in GNOME projects. It provides four new classes: - ShellTestFrameworkRegister: A singleton class accessible from a binding language used to register any object that implements ShellTestFrameworyDiscovery - ShellTestFrameworkDiscovery: An interface which describes a test suite that can be set up, torn down and be interrogated for any tests available on it (eg, introspection) and run a test as specified by a string. - ShellTestFrameworkFixture: A base class for test fixtures. Useful if you want to have a fixture hierarchy and chain up set up / tear down operations. - ShellTestFrameworkFreestandingTest: An abstract class implementing ShellTestFrameworkDiscovery with abstract methods to delegate to a subclass how tests should be run. The name of the suite is always the 'file-name' property and its discover_tests method always return a strv of size 1 with 'test-name' as the first element. Effectively, the way it works is that we provide some inline javascript before importing files that enables us add quick implementations for ShellTestFrameworkDiscovery's methods which interrogate a passed object for methods beginning with 'test' and provides them on request. It also searches for any functions beginning with 'test' and adds them as freestanding tests. Exceptions are caught at the boundary and turned into appropriate error messages. This mainly allows two things: 1. Integration with GTest, which means that there is a single test runner for a project. 2. A simple way to write introspectable test fixtures, which means that set-up and tear-down can be done from the C side if need be and then re-used by javascript tests.
Created attachment 263607 [details] [review] Rewrite format.js tests to use fixtures
Created attachment 263608 [details] [review] Rewrite insertSorted tests to use test fixtures
Created attachment 263609 [details] [review] Rewrite jsParse tests to use test fixtures
Created attachment 263610 [details] [review] Rewrite markup tests to use test fixtures
Created attachment 263611 [details] [review] Rewrite URL tests to use test fixtures
Created attachment 263612 [details] [review] Move some UI code out of the general testable functions. Also eliminate some calls to Environment.init, which appear to be fairly useless.
Created attachment 263613 [details] [review] Rewrite format.js tests to use test fixtures
Ugh, sorry for the spam everyone. Accidentally managed to hit "submit bug" instead of attaching more patches before hitting submit. That should be everything. I'm probably only looking for feedback at a high level - eg, is this even the right direction to go for this? I get the impression that the consensus is "it doesn't matter as long as the tests are running again". Feel free to offer detailed feedback too. I haven't written any GObject in well over a year now so I'd imagine that I'm a little behind the times in terms of best practices. Also, I'm pretty new to javascript.
Review of attachment 263604 [details] [review]: ::: js/misc/util.js @@ +7,3 @@ const St = imports.gi.St; +const Config = imports.misc.config; Why do you need config? @@ +154,3 @@ +function _notifyError(msg, details) { + Extra whitespace. @@ +156,3 @@ + + if (details) { + log("error: " + msg + ": " + details) Missing semi. Since the title also has "Execution of '%s' failed:", I don't think we need the "error: " prefix.
Review of attachment 263603 [details] [review]: Can this not go in misc/util.js for some reason? You also forgot to add this file to gresource.xml.in.
Review of attachment 263607 [details] [review]: Obsoleted by future patch.
Review of attachment 263613 [details] [review]: Can we move this test to gjs, considering the format code is now there?
Review of attachment 263606 [details] [review]: I'm fine with prototyping this in gnome-shell, but this should probably also be in gjs at some point. Just looking at the test runner for now. I'll do a more detailed review once we clean this up. ::: src/discover-js-unit-tests.c @@ +18,3 @@ +} ShellTestFrameworkGjsData; + +static ShellTestFrameworkGjsData * test_data_allocate (ShellTestFrameworkDiscovery *discovery, Bad style. We usually do: static ShellTestFrameworkGjsData * test_data_allocate (...) { } with the function name on a new line. Please fix the rest of the function declarations as well... @@ +52,3 @@ + if (!ret) + { + g_printf("Test %s failed.\n Assertion %s failed for reason %s.\n Relevant data: %s\n", g_printf? Ugh, that will go to stdout, not stderr. Is that all gtester gives us? (Missing a space between the function name and opening paren as well) @@ +87,3 @@ + gchar *import_filename = g_strndup (filename, strlen (filename) - 3); + gchar *test_file_import_script = + g_strconcat ("let TestFile = imports.", import_filename, ";\n", NULL); You do realize we can do stuff like: "let TestFile = imports[importFilename];", right? @@ +90,3 @@ + + static const gchar *discovery_script_header = + "Object.keys (TestFile).forEach (function (key) {\n"\ The backslashes at the ends of lines are not necessary. And ugh, I'd really prefer this to be in a generic JS script. @@ +93,3 @@ + " if (typeof TestFile[key] === 'function') {\n"\ + " if (typeof TestFile[key]._IsGJSTestFixture !== 'undefined') {"\ + " eval ('register.discoverable (new TestFile.' + key + ' ());');\n"\ let cls = TestFile[key]; register.discoverable(new cls()); @@ +242,3 @@ + + static const gchar *script = + "const STF = imports.gi.ShellTestFramework;\n"\ Why isn't this script in a .js file?
Review of attachment 263612 [details] [review]: I think we want to be careful about the Environment.init() stuff. Right now it's a series of random things to set up the expectations of our environment. I'd love to reduce our dependency on window.global, and if you have any ideas about that, let me know. _makeLoggingFunc is a quick hack -- I'd rather see a logging function that takes multiple arguments in gjs itself. I'm not sure about Gettext, but not having it in the tests seems wrong to me. Tests should be allowed to rely on code that uses translations. _patchContainerClass, I think we eventually want to move away from child properties in general, so those can eventually go. String.prototype.format = Format.format; should be in in tests. The rest of it (Tweener, slowdown factor, monkey-patching for debugging utilities) should be only done in a UI environment. I think we should probably move Gettext and String.prototype.format to another "initNonUI" which init() and tests will call. ::: js/misc/actorVisibility.js @@ +1,1 @@ +// -*- mode: js; js-indent-level: 4; indent-tabs-mode: nil -*- Should be called scrollViewUtils or something. "actorVisibility" is a bit generic for a scroll-view-specific function. @@ +1,3 @@ +// -*- mode: js; js-indent-level: 4; indent-tabs-mode: nil -*- + +const Tweener = imports.ui.tweener; This should probably be in imports.ui. The general rule, which we're really quite bad about following, is that imports.misc should not import code in imports.ui. ::: js/misc/closeButton.js @@ +1,1 @@ +// -*- mode: js; js-indent-level: 4; indent-tabs-mode: nil -*- This should be in imports.ui as well. @@ +53,3 @@ +}); + +function makeCloseButton(boxpointer) { We don't need makeCloseButton anymore. Just change callers to use the constructor.
Review of attachment 263608 [details] [review]: Looks a lot better.
Review of attachment 263609 [details] [review]: Looks OK. ::: tests/unit/jsParse.js @@ +31,3 @@ + +function testParseForMatchingQuoteForEndOfString() { + Extra whitespace at the beginning of functions/blocks. Please try not to do this!
Review of attachment 263609 [details] [review]: (wrong status)
Review of attachment 263610 [details] [review]: OK.
Review of attachment 263611 [details] [review]: OK.
Review of attachment 263606 [details] [review]: The only other thing here is the commit message. As a style nit, we don't tend to add a "." to the end of commit messages, and we tend to use the present tense for the subject: "Add framework for providing introspectable test fixtures for GTest"
Created attachment 263642 [details] [review] fixup! Reimplement notifyError in util.js.
Created attachment 263643 [details] [review] fixup! Rewrite format.js tests to use test fixtures
Created attachment 263644 [details] [review] fixup! Added a framework for providing introspectable test fixtures for GTest.
Created attachment 263645 [details] [review] fixup! Move some UI code out of the general testable functions.
Created attachment 263646 [details] [review] fixup! Move some UI code out of the general testable functions.
Created attachment 263647 [details] [review] fixup! Move some UI code out of the general testable functions.
Created attachment 263648 [details] [review] fixup! Rewrite jsParse tests to use test fixtures
Created attachment 263649 [details] [review] fixup! Added a framework for providing introspectable test fixtures for GTest.
Hey Jasper, I've added some fixup patches (nevermind the strikeous ... I guess bugzilla wasn't designed to deal with this). Please let me know if you'd rather have the rebased set instead - some people prefer fixups to rebases. I've incorporated most of your suggestions. The only (big) one I haven't incorporated so far was moving the inline script out into its own file. That's mainly because I don't know a good place to put it yet. One big change is that I've moved most of the logic out of discover-js-tests into ShellTestFramework (we need a better name for that), with the eventual aim of being able to specify the paths to the directories that we want to test in the binary instead of as command line options. Still an RFC of course. I still have no idea why the format tests are failing, though, as you say, maybe its a good time to drop them and move them to upstream Gjs.
Review of attachment 263642 [details] [review]: So, I also want to double-check on this. Why do we need this? I agree that the message tray isn't the best place for errors like this, but I think it was intended that something gets shown to the user on errors like this. If it's to make sure that importing util doesn't import from ui, I'd rather see a js/ui/spawnWithUi.js (bad name) or similar. ::: js/misc/util.js @@ +151,2 @@ if (details) { + log(msg + ": " + details) Missing semis.
Review of attachment 263643 [details] [review]: As you were saying on IRC, util.js is sort of a bad "oh we don't have any other place for this, let's slap it in util.js". This seems mostly harmless, except "markup" is sort of generic. Should we rename it to "fixMarkupForMessageTray" at the same time?
Review of attachment 263647 [details] [review]: ::: js/ui/environment.js @@ +63,3 @@ } +function coreInit() { We probably should have this stuff in a js/misc/environment.js, so we don't import Shell/St/Clutter/Tweener, etc. We should also make js/extensionPrefs/main.js use that as well instead of the fake thing it has going on. @@ -115,2 @@ // OK, now things are initialized enough that we can import shell JS - const Format = imports.format; Yikes, this comment has always been wrong for imports.format. We can put that import line at the top with the rest of the imports section. ::: tests/unit/format.js @@ +15,1 @@ String.prototype.format = Format.format; coreInit(); does this now. (and this test should be upstream)
Review of attachment 263648 [details] [review]: Thanks!
Review of attachment 263649 [details] [review]: This fixup is kind of hard to read. Can you squash and provide a new patch?
(In reply to comment #33) > Review of attachment 263642 [details] [review]: > > So, I also want to double-check on this. Why do we need this? I agree that the > message tray isn't the best place for errors like this, but I think it was > intended that something gets shown to the user on errors like this. > > If it's to make sure that importing util doesn't import from ui, I'd rather see > a js/ui/spawnWithUi.js (bad name) or similar. Yeah, the main reason I pulled it out was because util was importing from UI. What I can do is just refactor so that we inject the error notify handler into the spawn function. Then you get the message tray notification for real world usage and a terminal output (or maybe even /dev/null) for tests. > > ::: js/misc/util.js > @@ +151,2 @@ > if (details) { > + log(msg + ": " + details) > > Missing semis. Whoops. Thanks :)
(In reply to comment #35) > Review of attachment 263647 [details] [review]: > > ::: js/ui/environment.js > @@ +63,3 @@ > } > > +function coreInit() { > > We probably should have this stuff in a js/misc/environment.js, so we don't > import Shell/St/Clutter/Tweener, etc. We should also make > js/extensionPrefs/main.js use that as well instead of the fake thing it has > going on. > > @@ -115,2 @@ > // OK, now things are initialized enough that we can import shell JS > - const Format = imports.format; > > Yikes, this comment has always been wrong for imports.format. We can put that > import line at the top with the rest of the imports section. Yep, will do. > > ::: tests/unit/format.js > @@ +15,1 @@ > String.prototype.format = Format.format; > > coreInit(); does this now. (and this test should be upstream) And I still need to figure out why it doesn't work :)
Created attachment 263891 [details] [review] fixup! Reimplement notifyError in util.js
Created attachment 263892 [details] [review] fixup! Rewrite format.js to use test fixtures
Created attachment 263893 [details] [review] fixup! Move some UI code out of the general testable functions
Created attachment 263894 [details] [review] [Rebase] Added a framework for providing introspectable test fixtures in GTest.
Thanks Jasper, I've implemented your suggestions. The only things I haven't done is moved fixMarkupForMessageTray into another file. It should probably just live in util.js until we figure out what goes with it. I think moving it at this point is somewhat out of scope for these patches. I've also rebased the GTest test framework, although I'd like to talk about potentially using something like Jasmine [1] instead. [1] http://pivotal.github.io/jasmine/
Review of attachment 263891 [details] [review]: Not the biggest fan, but I can't think of anything better, so OK...
Review of attachment 263892 [details] [review]: OK.
Review of attachment 263893 [details] [review]: Can you make extensionPrefs/main.js use this too?
Review of attachment 263893 [details] [review]: Also, can you just make it init(); instead of coreInit(); ?
Review of attachment 263894 [details] [review]: My first thought is "why isn't more of this written in JS?" I can't see any reason that ShellTestFrameworkFixture or ShellTestFrameworkFreestandingTest need to be written in C. In fact, ShellTestFrameworkFixture seems mostly blank to me. My second thought is "do we really need all this complexity?" What's wrong with the jsUnit system that we have in gjs? It's not perfect, but it's workable. I really don't see what this buys us. It seems really complicated for no reason that I can see. ::: src/discover-js-unit-tests.c @@ +1,1 @@ +/* discover-js-unit-tests.c */ This needs a copyright header. ::: src/gnome-shell-js-test-runner.c @@ +36,3 @@ +}; + +static void shell_test_framework_js_test_runner_initable_interface_init (GInitableIface *interface); As I said before, I think this could be and should be written entirely in JS. I don't necessarily care where you put it -- if it's tests/jsTestDiscovery.js or similar. @@ +197,3 @@ + " Name: 'Discoverable' + params.Name,\n" + " Extends: FixtureClass,\n" + " Implements: [ STF.Discovery ],\n" I really don't like this amount of metaprogramming. Why do we need to create classes and objects to just run a function? @@ +202,3 @@ + " },\n" + " vfunc_fixture_set_up: function() {\n" + " this.vfunc_set_up ();\n" Huh? You shouldn't be calling foo.vfunc_blah(); directly at all. @@ +211,3 @@ + " },\n" + " vfunc_discover_tests: function() {\n" + " let available_tests = []\n" availableTests, and missing semi. ::: src/gnome-shell-test-discovery-iface.c @@ +24,3 @@ + +static void +shell_test_framework_discovery_default_init (ShellTestFrameworkDiscoveryInterface *iface); I'm not sure that "Discovery" is the best name for it, considering it's mostly a basic fixture. @@ +38,3 @@ + * + * Calls through the discoverable test fixture to its actual + * set_up function, if provided. I have no clue what this means. @@ +40,3 @@ + * set_up function, if provided. + * + * Since: 0.1 No need for Since tags. @@ +153,3 @@ + ShellTestFrameworkDiscovery *discovery) +{ + shell_test_framework_register_discoverable (reg, discovery); What's the point of this wrapper method? ::: src/gnome-shell-test-discovery-iface.h @@ +50,3 @@ + void (* fixture_tear_down) (ShellTestFrameworkDiscovery *discovery); + + const gchar * (* suite_name) (ShellTestFrameworkDiscovery *discovery); This should be get_suite_name ::: src/gnome-shell-test-register.c @@ +110,3 @@ +{ + /* Once everyone has unreff'd, the singleton is gone */ + reg_singleton = NULL; Will we ever use this? @@ +132,3 @@ + * singleton exists yet. If it doesn't, then create it + * by calling the constructor. If it does, then we can + * just ref the singleton and return */ GObject constructors should not do things like this. Simply create a get_default method like we do elsewhere. Also, is there any requirement that this be multithreaded? I think we can simply do like we do in e.g. shell_app_system_get_default() and use a static variable for this. @@ +169,3 @@ + * Return value: (transfer full): The instance of the #ShellTestFrameworkRegister + * + * Since: 0.1 We don't need a Since: tag for the Shell library.
> My first thought is "why isn't more of this written in JS?" I can't see any > reason that ShellTestFrameworkFixture or ShellTestFrameworkFreestandingTest > need to be written in C. In fact, ShellTestFrameworkFixture seems mostly blank > to me. > > My second thought is "do we really need all this complexity?" What's wrong with > the jsUnit system that we have in gjs? It's not perfect, but it's workable. > > I really don't see what this buys us. It seems really complicated for no reason > that I can see. I somewhat agree with you - hence the allusions I was making earlier as to this perhaps not being the right way to go. (Though I kinda wished we had discussed this a lot earlier in the review rather than it coming up now). The architecture is mostly the way it is now because we have GTest as our test runner. If were happy to drop that, then I think it could be a lot simpler. The main reason why I have the fixtures in C is so that they can be introspectable and we can subclass them either in C or JS to have common set up and tear down functionality for certain classes of tests. This means that if we need to do a bit of set up and tear down with an API that is not introspectacle then you only need to subclass the fixture itself and make that introspectable (easy) and we don't need to make the entire API introspectable. Its not useful *right now* but will become useful when we want to start doing acceptance tests and need to interrogate GL, Xlib or Wayland directly. As for the Discovery interface, that's basically just a bridge so that we can implement in a high level language and then run the tests without leaking language implementation details to the test runner. All it provides is a means to determine which tests live in a fixture and run specified tests in that fixture. We can then use it directly with GTest without having to eval () stuff on each test. All that said, the discovery interface is probably not necessary if we use a testing framework such as Jasmine for gnome-shell. I'd far prefer that approach, as long as we're happy to accept a dependency on jasmine which can be brought in as a submodule. We can then extend fixtures from C *if need be* As for JSUnit - my main beef is that is that it assumes each test is an independent script. There's also no test fixtures whatsoever so you can't share common set up and test down logic. This violates two principles of maintainable testing: 1. Lots of repetition of set up and tear down logic, which is bad for obvious reasons. 2. You should only test one thing per test. That he generally means you should only ever have one assert in each test. Having multiple asserts means that when a test fails you have to spend more time debugging it because it is testing too much. > > ::: src/discover-js-unit-tests.c > @@ +1,1 @@ > +/* discover-js-unit-tests.c */ > > This needs a copyright header. > > ::: src/gnome-shell-js-test-runner.c > @@ +36,3 @@ > +}; > + > +static void shell_test_framework_js_test_runner_initable_interface_init > (GInitableIface *interface); > > As I said before, I think this could be and should be written entirely in JS. I > don't necessarily care where you put it -- if it's tests/jsTestDiscovery.js or > similar. > > @@ +197,3 @@ > + " Name: 'Discoverable' + params.Name,\n" > + " Extends: FixtureClass,\n" > + " Implements: [ STF.Discovery ],\n" > > I really don't like this amount of metaprogramming. Why do we need to create > classes and objects to just run a function? As for hierarchy - I would agree that class hierarchies usually suck. However, they are well suited to this problem, because one is more concerned with reusing code and data than creating implementations of interfaces. In most tests you want to share as much of the set up and tear down logic as possible, so that you can have tests that look like this: TestFoo: # 1. Do something with foo. # 2. Assert something about foo. Its super-concise. All of the fixture logic gets pushed into the set up and test down functions. For most fixtures, they can usually be described as an extension of another, eg, create the object in X state and also mutate Y state. So you have a fixture that gets the object in X state, tests for that fixture and then a fixture that gets the object in X|Y state , which inherits from the former fixture and after chaining up in setUp, does whatever is needed to also get the object or environment in Y state. Now that I've braindumped a little, I'd probably like to end my commentary there and note that we're at a crossroads as to the desired architecture for all of this and so we should probably discuss on IRC what the best implementation would look like. > GObject constructors should not do things like this. Simply create a > get_default method like we do elsewhere. The documentation for GObject explicitly says that you should do this and so it should change if it is no longer considered a best practise.
It's safe to say that this bug was superseded by bug 783738, so closing. *** This bug has been marked as a duplicate of bug 783738 ***