@InjectBundleContext lifecycle problems

timothyjward commented 4 years ago

Background

The "normal" way to use the bnd-testing-maven-plugin is to package up all the unit tests and OSGi tests in a single test jar which is a fragment of the bundle being tested. This allows the unit tests to be in the same package(s) as the classes under test and therefore default visibility methods can be tested. The consequence of this is that in the OSGi tests the BundleContext for the tester bundle is the same as for the bundle being tested.

The Problem

In some of my OSGi tests I need to ensure a "clean" start of the bundle under test to avoid residual state (e.g. historical monitoring data). The simplest way to do this is to restart the bundle under test. This invalidates the BundleContext injected into my test and creates a new one. To ensure that I don't use a stale BundleContext I inject a new one into my test class using an @BeforeEach method, unfortunately the object my test is injected with appears to be the old invalid BundleContext and throws Exceptions when I try to use it in the next test.

Proposed solution

Each time the BundleContext is injected into a lifecycle it should be obtained from the tester bundle, and not cached and reused.

kriegfrj commented 4 years ago

Background

The "normal" way to use the bnd-testing-maven-plugin is to package up all the unit tests and OSGi tests in a single test jar which is a fragment of the bundle being tested. This allows the unit tests to be in the same package(s) as the classes under test and therefore default visibility methods can be tested. The consequence of this is that in the OSGi tests the BundleContext for the tester bundle is the same as for the bundle being tested.

I wouldn't go so far as to call this the "normal" way, but it is certainly a valid and common use case. Another approach I've used is to create a separate bundle, which makes it more of a black box test.

The Problem

In some of my OSGi tests I need to ensure a "clean" start of the bundle under test to avoid residual state (e.g. historical monitoring data). The simplest way to do this is to restart the bundle under test. This invalidates the BundleContext injected into my test and creates a new one. To ensure that I don't use a stale BundleContext I inject a new one into my test class using an @BeforeEach method, unfortunately the object my test is injected with appears to be the old invalid BundleContext and throws Exceptions when I try to use it in the next test.

Proposed solution

Each time the BundleContext is injected into a lifecycle it should be obtained from the tester bundle, and not cached and reused.

Thanks for highlight this use case. I want to make sure I understand it properly. If you could put a small example somewhere on Github where we could see it that might be easier.

You'll have to forgive me here as I'm not as experience with the finer points of OSGi as the others are. But one problem that I can see here is that (correct me if I'm wrong) you're trying to restart the bundle under test (BUT) ... but the test class is in a fragment that is attached to the BUT. I'm not quite sure how this is going to be able to work. Even if you make your test case into a standalone bundle, if you need to restart the BUT, this will also restart the test bundle if it is wired to the BUT.

You could try using reflection. That way you can force it to re-load the class object on every test after the bundle has re-loaded.The BuildpathQuickFixProcessorTest in Bndtools uses reflection to instantiate the class-under-test (which is in a private package), for example: https://github.com/bndtools/bnd/blob/9ba61b3cab5145c5cf036e4fd747272fc2b11940/bndtools.core.test/src/bndtools/core/test/editors/quickfix/BuildpathQuickFixProcessorTest.java#L117-L129 It loads it once in beforeAll() but there's no reason it couldn't be loading it in beforeEach().

I hope that I've not gone off on a wild tangent and that this actually helps...

timothyjward commented 4 years ago

If you could put a small example somewhere on Github where we could see it that might be easier.

Fortunately this is part of a test suite in Apache Aries, so it's easy for me to share:

You can see the workaround I've used here. At the start I convert the Context into a bundle and then get the bundle context from the bundle each time I need it (shielding me from the start/stop happening here)

My original approach was a bit different - I injected the BundleContext into the beforeEach method, but that resulted in all tests failing after the first one, due to an "invalid bundle context".

kriegfrj commented 4 years ago

Ok, I had a discussion about this with @timothyjward and I've looked at his example, and pondered it over the past weeks. I'd really like @bjhargrave or @rotty3000 's input into this as their OSGi experience is much greater than mine, but...

In short, I don't think that this usage pattern will be workable. My understanding of OSGi is that for a well-behaved bundle, any references to classes that belong to that bundle should be forgotten ASAP once the bundle is stopped. This usage pattern violates this principle.

I think the better approach is to decouple your test bundle from the bundle-under-test so that restarting the latter does not force a restart of the former. That way, your tests will be free to restart the bundle-under-test in @BeforeEach without forcing the test bundle to be restarted, so the test bundle's context (which is the BundleContext used by the BundleContextExtension) will be preserved. To do this, you'll need to make sure that there are no direct dependencies on the bundle-under-test (ie, no Fragment-Host, no Import-Package, etc). If you need direct access to the classes of the bundle-under-test, then use reflection to get the references to the classes and make sure that re-load the classes in @BeforeEach after you've restarted the bundle.

Thoughts gents?

kriegfrj commented 4 years ago

In the previous post I provided a workaround Just thinking a little bit further about how we could tackle this sort of problem in a consistent way as a part of osgi-test.

Thought bubble 1: We revisit #65 (which I tried to rename to more accurately reflect what I think is the intent), to allow an option that restarts the framework on every test run. This may simply degenerate into something indistinguishable from Launchpad though.
Thought bubble 2: We come up with a BundleExtension (name up for negotiation) which helps to automate the bundle-restarting and reflection. Here's a rough syntax:


@ExtendWith(BundleExtension.class)
class MyTestClass {

  @InjectBundle(value = "my.bundle.bsn", restart = true)
  Bundle myBundle;

  @InjectClass(bundle = "my.bundle.bsn", clazz = "my.fq.class.Name")
  Class<? extends CommonSupertype> myFQClassName;

}

Now BundleExtension.beforeEach() will restart bundle my.bundle.bsn, and then use the bundle's classloader to load my.fq.class.Name, check that it is assignment compatible with the wildcard bound (CommonSupertype), and then inject it.

As a convenient piece of syntactic gloss, the "bundle" parameter of @InjectClass could be inferred if there is only one @RestartBundle annotation on the test class.

Thoughts?

rotty3000 commented 4 years ago

Whenever I have needed to "test" bundle lifecycle effects I've used the bnd $make function to generate embedded bundles. This way the test code and the bundle under test are always seperate constructs isolating their state from each other. It's a little tedious, but one the recipe is in place and with the InstallBundle utility it's not too difficult.

Take a look at her Aries CDI project from an example of this usage in a maven project.

On Tue., Oct. 13, 2020, 2:25 a.m. Fr Jeremy Krieg, notifications@github.com wrote:

In the previous post I provided a workaround Just thinking a little bit further about how we could tackle this sort of problem in a consistent way as a part of osgi-test.

Thought bubble 1: We revisit #65 https://github.com/osgi/osgi-test/issues/65 (which I tried to rename to more accurately reflect what I think is the intent), to allow an option that restarts the framework on every test run. This may simply degenerate into something indistinguishable from Launchpad though.

Thought bubble 2: We come up with a BundleExtension (name up for negotiation) which helps to automate the bundle-restarting and reflection. Here's a rough syntax:

@ExtendWith(BundleExtension.class)class MyTestClass {

@InjectBundle(value = "my.bundle.bsn", restart = true) Bundle myBundle;

@InjectClass(bundle = "my.bundle.bsn", clazz = "my.fq.class.Name") Class<? extends CommonSupertype> myFQClassName;

}

Now BundleExtension.beforeEach() will restart bundle my.bundle.bsn, and then use the bundle's classloader to load my.fq.class.Name, check that it is assignment compatible with the wildcard bound (CommonSupertype), and then inject it.

As a convenient piece of syntactic gloss, the "bundle" parameter of @InjectClass could be inferred if there is only one @RestartBundle annotation on the test class.

Thoughts?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/osgi/osgi-test/issues/212#issuecomment-707518731, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABD2TCIDDL4GZXTDJPSB33SKPXE3ANCNFSM4RRZNZVQ .

timothyjward commented 3 years ago

For reference, I do think that this would be an easy issue to fix. These two methods: https://github.com/osgi/osgi-test/blob/c69436b6d6bf9e5aa624a89ca1948077bb443055/org.osgi.test.common/src/main/java/org/osgi/test/common/context/CloseableBundleContext.java#L134-L142 should be deprecated (or removed) and replaced with similar methods that receive a Bundle. The Bundle should be saved, and every time a context is needed then it should be obtained from that bundle. This will prevent the CloseableBundleContext from caching an invalid bundle context if the tester bundle is stopped and started part way through the test.

bjhargrave commented 3 years ago

I wouldn't go so far as to call this the "normal" way, but it is certainly a valid and common use case.

If you need to change the start/stop state of the host bundle, then I would argue that using a test fragment is not a valid way to test. That is like a bundle trying to start and stop itself which is basically undefined.

kriegfrj commented 3 years ago

If you need to change the start/stop state of the host bundle, then I would argue that using a test fragment is not a valid way to test. That is like a bundle trying to start and stop itself which is basically undefined.

I concur. Shall we close this issue?

osgi / osgi-test