Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring so GWT unit tests run less slowly #4460

Merged
merged 10 commits into from Mar 18, 2019

Conversation

gtritchie
Copy link
Member

The GWT unit tests (ant unittest) was taking over a minute to run each time, making it painful to use for... unit testing.

The problem (besides the inherent slowness of GWTTestCase and its need to run a hidden browser) are classes internally invoking dependency injection, such as RStudioGinjector.INSTANCE.injectMembers(this) or RStudioGinjector.INSTANCE.getCommands()... and so on.

By doing this, they were dragging in pretty much the entire codebase for each test in order to perform the injection of the real object set. This is (very) slow to startup, and also goes against the very notion of unit testing (and defeats a big reason for using the dependency injection framework) as these tests are essentially doing integration testing of many classes at once instead of testing a single class in a well-controlled manner.

The goal, then is to minimize calls to the injector, and instead rely on injection via constructors, so tests can supply mock/fake alternatives.

VirtualConsole

The VirtualConsole class, when constructed, was injecting UIPrefs into itself so it could react to a couple of preferences. I fixed this by defining an interface, VirtualConsole.Preferences, which defines just the accessors needed by VirtualConsole. That interface is injected via assisted injection to the VirtualConsole constructor. At product runtime, Gin injects an instance of a new VirtualConsolePreferences object implementing that interface (which itself gets the actual UIPrefs object injected); in the tests, where dependency injection is not used, we define and pass in our own fake implementation of the interface.

Assisted injection allows injection where the constructor needs some arguments provided by the caller, along with those being injected. This requires wiring up and using a Gin factory interface; see VirtualConsoleFactory.

Several places in the production code that create VirtualConsole's went from this pattern:

virtualConsole_ = new VirtualConsole(output_.getElement());

to

virtualConsole_ = RStudioGinjector.INSTANCE.getVirtualConsoleFactory().create(output_.getElement());

ConsoleOutputWriter

This class internally allocates a VirtualConsole object (originally via new); now it is passed the VirtualConsoleFactory interface and uses that. The sole caller of ConsoleOutputWriter is ShellWidget which isn't currently under test, so kicked the can down the road and let it do the dependency injection of the factory via getVirtualConsoleFactory(). This lets us test ConsoleOutputWriter itself by supplying it our own fake VirtualConsoleFactory.

JobsList / JobItem

These classes are UI widgets that also contain quite a bit of logic; ideally I'd separate them into a cleaner model/view/presenter model and focus on testing the presenter, but I had written tests directly against them and doubt I'll ever get to that exercise.

JobItem objects are allocated by JobsList, and then using RStudioGinjector.INSTANCE to get at the EventBus and at a "stop" command icon. Thus bogging down the tests.

Again I implemented the assisted injection pattern on them, and pass in a new interface FireEvents (extracted from EventBus); in production code this is injected by Gin as EventBus; in test I can pass in my own fake implementation. For getting at the icon, I just used the one that had already been added but not referenced and avoided the need to abstract Commands.

With all of this the unit tests are running in about 15 - 20 seconds. Suspect improving this would require moving some tests to be pure Java / JUnit, and additional abstraction to reduce amount of UI code being tested.

Pro will require additional work (it has a couple of extra Pro-only tests); those will still be slow, but this change can be merged into Pro as-is.

… during unit tests

- this is part of a series of refactorings needed to make `ant unittest` not take FOREVER
- unit tests should not be using GIN under the covers, all arguments should be supplied by the unit tests, both for performance reasons and for testability reasons
…to client

- core isn't supposed to depend on client, though this is far from the only offender
@gtritchie gtritchie added this to the v1.3 milestone Mar 15, 2019
@gtritchie gtritchie merged commit bfa2350 into v1.3 Mar 18, 2019
@gtritchie gtritchie deleted the bugfix/speed-up-gwt-unittests branch March 18, 2019 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant