Elegant way to define additional or overriding Guice bindings in DSL tests

[HannesWell]

For the test plugin of our custom DSL I want to define extra bindings for the Guice-Injector used to inject objects in the test.

Since the usual setup in DSL test classes using JUnit-5 is something like

@ExtendWith(InjectionExtension.class)
@InjectWith(MyDSLInjectorProvider.class)
public abstract class DC43BaseApplyTest {
  <the test code>
}

The approach to achieve that is described in this Stack-Overflow question and basically suggest to extend the generated MyDSLInjectorProvider by overriding its createRuntimeModule() method and return a customized Module that adds the desired bindings. The extended provider can then be used in the test classes.

This all works well (AFAICT) but it is a bit inconvenient because by default the InjectorProvider is generated as follows:

public class MyDSLInjectorProvider implements IInjectorProvider, IRegistryConfigurator {

...
	protected MyDSLRuntimeModule createRuntimeModule() {
		// make it work also with Maven/Tycho and OSGI
		// see https://bugs.eclipse.org/bugs/show_bug.cgi?id=493672
		return new MyDSLRuntimeModule() {
			@Override
			public ClassLoader bindClassLoaderToInstance() {
				return MyDSLInjectorProvider.class
						.getClassLoader();
			}
		};
	}
...
}

Because the generated createRuntimeModule() returns the specific module type generated for the DSL a sub-classes that want to override that method has to return the specific type as well and consequently has to repeat the ceremony to make the injector work in Maven/Tycho and OSGI or one has to override internalCreateInjector() as it is done in

xtext/org.eclipse.xtext.testlanguages.ide/src/org/eclipse/xtext/testlanguages/xtextgrammar/ide/XtextGrammarTestLanguageIdeInjectorProvider.java

Lines 24 to 35 in 73bd158

	protected Injector internalCreateInjector() {
	return new XtextGrammarTestLanguageStandaloneSetup() {
	@Override
	public Injector createInjector() {
	return Guice.createInjector(Modules2.mixin(createRuntimeModule(), createIdeModule()));
	}
	private XtextGrammarTestLanguageIdeModule createIdeModule() {
	return new XtextGrammarTestLanguageIdeModule();
	}
	}.createInjectorAndDoEMFRegistration();
	}

But this also includes repeating ceremony.

If createRuntimeModule() would declare its return type to be just com.google.inject.Module then a customized provider could be just

public class MyDSLExtendedInjectorProvider extends MyDSLInjectorProvider {
	@Override
	protected Module createRuntimeModule() {
		return Modules.override(super.createRuntimeModule()).with(binder -> {
			binder.bind(SomeInterface.class).to(TestImplementationOfSomeInterface.class);
		});
	}
}

It would be even simpler if the generated InjectorProvider would have for example a protected Module createTestModule() that by default just returns null and can be overriden in subclasses:

public class MyDSLInjectorProvider implements IInjectorProvider, IRegistryConfigurator {

...
	protected Injector internalCreateInjector() {
		return new MyDSLStandaloneSetup() {
			@Override
			public Injector createInjector() {
				Module testModule = createTestModule();
				return Guice.createInjector(testModule != null 
					? Modules.override(createRuntimeModule()).with(testModule)
					: createRuntimeModule());
			}
		}.createInjectorAndDoEMFRegistration();
	}

	protected MyDSLRuntimeModule createRuntimeModule() {
		... as before
	}
	protected Module createTestModule() {
		return null;
	}
...
}

A customized InjectorProvider would then just be

public class MyDSLExtendedInjectorProvider extends MyDSLInjectorProvider {
	@Override
	protected Module createTestModule() {
		return binder -> {
			binder.bind(SomeInterface.class).to(TestImplementationOfSomeInterface.class);
		};
	}
}

Given there are no other classes in the package calling createRuntimeModule() both approaches should be source compatible with respect to existing sub-classes (a subclass can narrow the return type), but the latter should be 'safer' if custom sub-classes call createRuntimeModule() and expect the specific Module type.

Do you know a better solution or do you think that would be a suitable enhancement of Xtext's template for the test InjectorProvider?

[cdietrich]

Is this about unit tests or plug-in tests

[LorenzoBettini]

Just a quick answer, more on that tomorrow.

@cdietrich that "trick" in the generated InjectorProvider is not required (but it's harmless) when running JUnit test from Eclipse where the classpath and the classloader are flat. It's required when you run those tests as plug-in tests, NOT necessarily requiring the UI. Since Tycho surefire always runs tests in OSGI, it's like running a plug-in test. In general, to reproduce the problems that the "trick" fixes is enough to run the test as a plug-in test, even without the UI.

@HannesWell in general I like your proposal because that "trick" added by me was a first step but I always knew that a custom injector provider requires the ceremony. I'd like to think about a possible solution in more depth, because I think we could also take the chance to improve also the "trick" of the classloader further.

[cdietrich]

@szarnekow wdyt

[szarnekow]

Initial thoughts:

I'm all in favor of making the generated injector provider more extensible.
I'm not sure we want the distinction between test-module and runtime-module in the API though. We only need the module, and the overriding method can do something like Modules.mixin(super.createModule(), binder->binder.bind(..)) manually. This would not suggest that the bindings are for tests specifically.

public class MyDSLInjectorProvider implements IInjectorProvider, IRegistryConfigurator {

	protected Injector internalCreateInjector() {
		return new MyDSLStandaloneSetup() {
			@Override
			public Injector createInjector() {
				return Guice.createInjector(createModule() );
			}
		}.createInjectorAndDoEMFRegistration();
	}

	protected Module createModule() {
		... as before
	}
}

public class MyDSLExtendedInjectorProvider extends MyDSLInjectorProvider {
	@Override
	protected Module createModule() {
		return Modules2.mixin(super.createModule(), binder -> {
			binder.bind(SomeInterface.class).to(TestImplementationOfSomeInterface.class);
		});
	}
}

We could even go one step further and allow
@InjectWith(value=MyDSLInjectorProvider.class, additionally=AnotherModule.class)

[LorenzoBettini]

I'm afraid I'm a bit busy today, so I'm unable to fully explain my idea now.

I'll try to give an hint: to fix the problem of the classloader to work in OSGI, it is enough to rely on the default implementation

xtext/org.eclipse.xtext/src-gen/org/eclipse/xtext/AbstractXtextRuntimeModule.java

Line 60 in 73bd158

public ClassLoader bindClassLoaderToInstance() {

, which is based on dynamic binding: if we generate another runtime module for testing purposes in the tests project, which only inherits from the runtime module of the main project, the classloader will be naturally picked correctly. I'm suggesting to use the generation gap pattern again.

[LorenzoBettini]

So, I remembered correctly: to make the classloading work it is enough to do like this in the generated injector provider:

protected MyDslRuntimeModule createRuntimeModule() {
	// make it work also with Maven/Tycho and OSGI
	// see https://bugs.eclipse.org/bugs/show_bug.cgi?id=493672
	// allows for bindClassLoaderToInstance to get the class loader of this bundle
	return new MyDslRuntimeModule() {
	};
}

I've just tested it in 4e695af

I didn't make it like that in the first place because Sven suggested to be more explicit (https://bugs.eclipse.org/bugs/show_bug.cgi?id=493672#c1):

Ok, but the ClassLoader binding should be done explicitly, so it is obvious why the code is how it is.

I think it's better to be "less explicit" nowadays ;) The "ceremony" mentioned by @HannesWell is not required, at least, there's no need to override bindClassLoaderToInstance. I guess he was misled by the shape of the currently generated injector provider. Instead of being "obvious" we are more "obscure" ;)

There's still one problem though if we plan to provide the additional customization options: if you derive from the injector provider in another test bundle, based on the test bundle of a test language (which we do, e.g., in the tests of 4e695af), you still have to redefine the createRuntimeModule so that the classloader of the current bundle is correctly taken. This could be avoided by making the generated createRuntimeModule completely rely on dynamic binding:

protected MyDslRuntimeModule createRuntimeModule() {
	// make it work also with Maven/Tycho and OSGI
	// see https://bugs.eclipse.org/bugs/show_bug.cgi?id=493672
	// allows for bindClassLoaderToInstance to get the class loader of this bundle
	return new MyDslRuntimeModule() {
			@Override
			public ClassLoader bindClassLoaderToInstance() {
				return MyDslInjectorProvider.this.getClass()
						.getClassLoader();
			}
	};
}

please let me stress it: I suggest to do

MyDslInjectorProvider.this.getClass().getClassLoader();

instead of the current

MyDslInjectorProvider.class.getClassLoader();

That way, the 4e695af could be further simplified (I've tested it locally by manually modifying the generated injector provider):

public static class XImportSectionTestLangInjectorProviderCustom extends XImportSectionTestLangInjectorProvider {
// nothing else to do here
}

In fact, the classloader would be taken from the class of this injector provider, using dynamic binding, that is, the most derived class, instead of statically from the class MyDslInjectorProvider. This would let us be "explicit" at the right dynamic level, I guess ;)

This would make the suggested improvements in customizations even easier to use.

@szarnekow what do you think?

[HannesWell]

I'm not sure we want the distinction between test-module and runtime-module in the API though. We only need the module, and the overriding method can do something like Modules.mixin(super.createModule(), binder->binder.bind(..)) manually. This would not suggest that the bindings are for tests specifically.

Yes, that would be totally fine for me as well and would be the approach with the least changes and yet greatest flexibility.
My second proposal was mainly to avoid some code for the specific test use-case to make it more convenient, but yes it would also be less flexible.

We could even go one step further and allow @InjectWith(value=MyDSLInjectorProvider.class, aditionally=AnotherModule.class)

Yes I thought about that as well but discarded it at least for my use-case since I would have to adapt each test case to use the customized module. Nevertheless I think it would be useful as complementary change to allow local customization's for specific tests only. Then one could for example define a custom module for a test-class just within that class as static inner class.

But to achieve that I think the generated InjectorProvider have to become cooperative and accept additional modules in their getInjector() method that are then eventually combined with the one generated for the DSL.
A first idea would be to introduce an IExtendedInjectorProvider interface whose getInjector() simply accepts a collection of additional modules which are passed to the generated internalCreateInjecto() method and combined with the module returned by createRuntimeModule().
Then the InjectExtension could check for that sub-class and report an error at runtime if additional modules are specified but the injector has not yet been regenerated.
One step further would be to have another InjectWithExtended annotation that just accepts the extended injector. But I'm not sure if this is too much to change (since there is probably a lot of documentation with the existing InjectWith annotation.

In fact, the classloader would be taken from the class of this injector provider, using dynamic binding, that is, the most derived class, instead of statically from the class MyDslInjectorProvider. This would let us be "explicit" at the right dynamic level, I guess ;)

One really has to think about it sharply and twice, but it should work as expected. 👍🏽 If one creates an instance of a sub-class of the generated InjectorProvider declared in another test-bundle, then the outer instance is of that sub-type and consequently the classloader of other (test-) bundle should be returned.

If you are fine with all the suggestions I can provide a PR to resolve this.