Comment 1 Bogdan Gheorghe 2012-02-06 22:23:36 EST

There seems to be a cycle in the classpath that was introduced by adding the discovery test feature.

To summarize:

The fragment o.e.p.d.test.unit has a host of o.e.p.d.util.

o.e.p.d.test.unit requires
  o.e.p.d.test.testutils which requires
   o.e.p.d.util 

If you have all of the projects loaded in the workspace everything looks OK but apparently things choke at build time.

There are only 5 lines of code that need the o.e.p.d.util, I've commented those out for now and removed the dependency on o.e.p.d.util from testutils. I've scheduled another build for 16:15 to see if this helps things.

Comment 2 Paul Webster 2012-02-07 08:56:26 EST

Apparently that's not quite enough:

Comment 3 Paul Webster 2012-02-07 09:04:05 EST

From the I20120206-1615 build:

genericTargets.xml:111: A cycle was detected when generating the classpath org.eclipse.demo.cheatsheets.search_0.11.0.v20110817-1130, org.eclipse.platform.discovery.core_0.11.0.v20110817-1130, org.eclipse.platform.discovery.runtime_0.11.0.v20110817-1130, org.eclipse.platform.discovery.util_0.11.0.v20110822-1145, org.eclipse.platform.discovery.test.testutils_0.11.0.v20120206-2102, org.eclipse.platform.discovery.runtime_0.11.0.v20110817-1130

Andrew, what's a starting point to try and debug why we're getting this during a build?  In our last iteration we had complaints because of the test fragments.  The fragments are not mentioned here, but I suspect they still contribute to the cycle.

i.e. every fragment requires the bundle org.eclipse.platform.discovery.test.testutils, and 
org.eclipse.platform.discovery.test.testutils requires org.eclipse.platform.discovery.runtime and org.eclipse.platform.discovery.ui (which create a cycle when combined with their fragments?)

Danail, the test fragments are causing our build a lot of problems.

PW

Comment 4 Danail Branekov 2012-02-07 10:31:39 EST

>Danail, the test fragments are causing our build a lot of problems.
Paul, do I understand correctly that there are general issues when using fragments to hold the automated tests and therefore test fragments are discouraged? Would converting test fragments to bundles solve the build issue?

Comment 5 Paul Webster 2012-02-07 10:42:15 EST

(In reply to comment #4)
> >Danail, the test fragments are causing our build a lot of problems.
> Paul, do I understand correctly that there are general issues when using
> fragments to hold the automated tests 

Yes, I was just getting to that write-up :-)

It looks like for a number of reasons, that using test plugins instead of fragments is the way to go.  The build cycles are one problem.  The automated test framework might not handle the fragments well either.

If you could convert them from test fragments to test plugins, we can move forward (and if you run into any problems, let us know).

PW

Comment 6 Dimitar Georgiev 2012-02-07 12:39:59 EST

Dani is working on temporarily excluding the Search Console tests from the build, for the purposes of not blocking the whole e4 build.

I would like to have a discussion on whether the solution proposed by Paul is the correct one.

- Using fragments has a number of advantages. The most notable one is that the whole classpath of the parent is visible in the tests, and so you never end up exposing internal stuff for test purposes. Also you need not double maintain the dependencies of the productive code, some of which invariably leak in the API you are testing.
All in all, we have always believed that this fragment approach is a positive pattern.

- Secondly, to my best knowledge, nothing in the OSGi spec forbids cyclic dependencies. It does not even matter if the component in question is a bundle or fragment bundle - neither resolution nor starting of the framework fails in this case. 

I would like to raise the following question:

- Based in the above, could we assume that the experienced behaviour is a defect in the build framework we are using?
- If so, what are the perspectives to have this fixed, and what is the effort?

Thank you!

Regards, Dimitar

Comment 7 Dimitar Georgiev 2012-02-07 12:42:19 EST

>> Dani is working on temporarily excluding the Search Console tests from the
build, for the purposes of not blocking the whole e4 build.

I am sorry, I misinformed you here. We do not know how to do this.
Can you help us temporarily exclude search console tests from the build?

Regards, Dimitar

Comment 8 Paul Webster 2012-02-07 12:44:43 EST

(In reply to comment #7)
> I am sorry, I misinformed you here. We do not know how to do this.
> Can you help us temporarily exclude search console tests from the build?

Bogdan's on it.

PW

Comment 9 Paul Webster 2012-02-07 13:05:10 EST

(In reply to comment #6)
> - Using fragments has a number of advantages. The most notable one is that the
> whole classpath of the parent is visible in the tests, and so you never end up
> exposing internal stuff for test purposes. Also you need not double maintain
> the dependencies of the productive code, some of which invariably leak in the
> API you are testing.

Depends what you mean by exposing "internals".  We have a policy in the Eclipse Project that all packages must be exported from our plugins.  They can be marked internal or contain "internal" (which auto-marks them as internal).  So having a test plugin as opposed to a test fragment doesn't interfere with the plugin configuration.

The only time this makes a difference is if your test has to test package private methods.  In the SDK we either up the visibility of some members (not that important on internals) or provide TestHelper classes that can be used by tests without change the visibility of the classes in question (core runtime and equinox take this approach).  In their tests we just ship the test helpers, but that can be avoided by providing a fragment to the host plugin that exposes the TestHelper method(s) in question to the test plugin (and with no fragment dependencies it avoids the cycle).

> 
> I would like to raise the following question:
> 
> - Based in the above, could we assume that the experienced behaviour is a
> defect in the build framework we are using?

Bug 309321 - Product build fails because of incorrect classpath cycle
Bug 181508 - [patch]Test framework cannot run tests provided by a fragment

Seems the test framework won't easily consume test fragments.

> - If so, what are the perspectives to have this fixed, and what is the effort?

PDE build is unmanned at the moment, so it is unfortunately unfixable.

But I would recommend against it.  All of our tests in the Eclipse Project are done via test plugins.  I wouldn't recommend introducing a new way even if there weren't the 2 above problems.

PW

Comment 10 Dimitar Georgiev 2012-02-08 04:22:21 EST

Hi Paul,

Thanks for the elaborate answer. Seems we are not the first to experience this issue.

The approach you described would work, and is a sort of solution to the problem. It has the downside of being coupled to equinox (x-internal,x-friends) or PDE ("internal" naming convention).

Also it is clear that "export all" was not the way meant by the OSGi module layer.

How much the coupling to equinox and PDE is an actual problem, and how much conforming to OSGi principles is an actual benefit, is a matter of subjective discussion. Nevertheless, after discussion with Danail, we are kind of reluctant to take on the approach you've pointed us at.

Can you please comment on an other possible solution - Bug 370922 

Thanks!

Regards, Dimitar

Comment 11 Paul Webster 2012-02-08 08:51:25 EST

(In reply to comment #10)
> The approach you described would work, and is a sort of solution to the
> problem. It has the downside of being coupled to equinox (x-internal,x-friends)
> or PDE ("internal" naming convention).

Sorry, I'm not sure what you are getting at here.  Your packages are API unless you mark them as internal (one way or the other).  All packages in your bundles *must* be exported.  That's a policy, not a best practice.

We're following both http://wiki.eclipse.org/Export-Package and http://wiki.eclipse.org/Version_Numbering (see http://wiki.eclipse.org/Category:API for more information).

That we use test plugins instead of test fragments is not a policy but a best practice.  That the tests can be built by PDE and run by the Eclipse automated test framework is a requirement, and apparently test fragments cannot meet that requirement.

> Can you please comment on an other possible solution - Bug 370922 

I'll put my full comments in the bug, but this is doable ... if it can meet a minimal set of usecases.

PW

Comment 12 Bogdan Gheorghe 2012-02-08 10:31:46 EST

This was resolved by backing out the discovery tests.

http://git.eclipse.org/c/e4/org.eclipse.e4.search.git/commit/?id=5305e2b190cc66b3deaa708a6ab54791f23da377