Add unit tests for Karaf features#516
Conversation
|
hi @googlielmo do you know there are pax exam tests for karaf in brooklyn-dist? If I recall correctly, the tests are there because they are testing the whole of brooklyn (not just brooklyn-server). |
|
hi @geomacy, yes I'm aware of that; as these are actually unit tests (not integration tests), the idea is to test the |
|
@googlielmo I guess that seems sensible, just wondered if you knew about them. thanks |
|
I should have clarified the (somewhat limited) scope of this PR in the first place, thank you for giving me the occasion to do it @geomacy 🙂 |
googlielmo
force-pushed
the
feature/test-karaf-features
branch
2 times, most recently
from
333f1b3 to
0371e50
Compare
googlielmo
changed the title
|
Rebased on master (not squashed) |
karaf/itests/pom.xml
Outdated
| <pax.exam.version>4.9.1</pax.exam.version> | ||
| <pax-url.version>2.5.2</pax-url.version> | ||
| <servlet.api.version>3.1.0</servlet.api.version> | ||
| <validation.api.version>1.1.0.Final</validation.api.version> |
There was a problem hiding this comment.
Can you move these to the parent pom
karaf/itests/pom.xml
Outdated
| <plugin> | ||
| <groupId>org.apache.servicemix.tooling</groupId> | ||
| <artifactId>depends-maven-plugin</artifactId> | ||
| <version>1.3</version> |
| --> | ||
| <!-- TODO use ServiceMix bundle for okhttp when dependency on okio is fixed there --> | ||
| <bundle dependency="true">wrap:mvn:com.squareup.okhttp/okhttp/2.2.0$Import-Package=org.apache.http.*;resolution:=optional,android.util.*;resolution:=optional,okio;version=%27[1.2.0,1.3.0)%27,*</bundle> | ||
| <bundle dependency="true">mvn:org.apache.servicemix.bundles/org.apache.servicemix.bundles.okhttp/2.2.0_2</bundle> |
There was a problem hiding this comment.
Can you explain your reasoning for this ... I remember there being an issue with the servicemix wrapped version
Also version should be parameterised
There was a problem hiding this comment.
Hi @m4rkmckenna servicemix accepted my pull request apache/servicemix-bundles#91 so it's safe to use their version now!
|
Thanks for the comments @m4rkmckenna. Will address them with a new commit. |
m4rkmckenna
left a comment
m4rkmckenna
left a comment
There was a problem hiding this comment.
Overall I can see value in having this ... but currently only tests if a feature installs correctly.
I would like to see some more assertions made on the health of the brooklyn kernel. ie Does the feature install cause a wave of bundle restarts
| <dependency> | ||
| <groupId>org.apache.geronimo.specs</groupId> | ||
| <artifactId>geronimo-atinject_1.0_spec</artifactId> | ||
| <version>1.0</version> |
| @RunWith(PaxExam.class) | ||
| @ExamReactorStrategy(PerClass.class) | ||
| public class FeatureInstallationTest extends TestBase { | ||
|
|
There was a problem hiding this comment.
Can you comment on how this differs from the verify phase of the karaf-maven-plugin
(Differentiator to karaf-maven-plugin) Should we be performing some sort of assertion after feature install?
ie can we establish the health of the karaf / brooklyn kernel?
There was a problem hiding this comment.
Had a look at this with @andreaturli who wrote the first unit tests in this PR.
It's fair to say that at the moment this is quite similar to the verify phase of the karaf-maven-plugin, especially as we're using the PerClass Exam reactor strategy.
At the same time, yes we can add more tests / assertions to check the health of karaf / brooklyn, good point! Will add some.
|
@googlielmo (cc @m4rkmckenna) given Marks' comment about seeing value in this, then I'm fine with us merging it and then iterating on it to add more assertions (as we find time to do that). That feels better than having the PR sitting around for longer. Does that sound sensible to you @m4rkmckenna? @googlielmo can you squash your commits please, and ensure there is a nice description on the commit(s)? (We want the commits descriptions to be understandable when glancing through the history of Brooklyn commits, where we can't easily tell which PR they were in; commit messages like "wip" don't tell us what it's for). |
core/pom.xml
Outdated
| <dependency> | ||
| <groupId>com.hierynomus</groupId> | ||
| <artifactId>sshj</artifactId> | ||
| <version>${sshj.version}</version> |
There was a problem hiding this comment.
Do we really need this line? It's defined int he dependencyManagement section of parent/pom.xml, so we shouldn't have to repeat it here.
| <servicemix.version>6.0.0</servicemix.version> | ||
| <reflections.bundle.version>0.9.9_1</reflections.bundle.version> <!-- see reflections.version --> | ||
|
|
||
| <maven.buildhelper.plugin.version>1.10</maven.buildhelper.plugin.version> |
| <maven.compiler.source>${java.version}</maven.compiler.source> | ||
| <maven.compiler.target>${java.version}</maven.compiler.target> | ||
|
|
||
| <okhttp.bundle.version>2.2.0_2</okhttp.bundle.version> |
There was a problem hiding this comment.
I wonder where this should be defined - should it be in the root pom.xml, alongside all the other dependency versions? Or should it be here (closer to where it's used in feature.xml)? I lean towards the former. But no strong feelings.
- test the brooklyn-server features at every build, to spot
regressions early on
- Rebase PR
- parameterise version numbers - remove stale TODO comment
googlielmo
force-pushed
the
feature/test-karaf-features
branch
from
dbae41d to
e058ced
Compare
|
@aledsage I squashed and edited the commit messages to something more useful. |
|
I agree that we should merge this and iterate, since there have been no more comments I'm going to do that now. Thanks @googlielmo @m4rkmckenna @aledsage |
7 participants
Add pax exam unit tests to brooklyn server / karaf.
Currently, the unit tests check that the Karaf features load correctly.