You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by hzbarcea <gi...@git.apache.org> on 2015/08/11 03:18:01 UTC
[GitHub] incubator-brooklyn pull request: Core test bundles
GitHub user hzbarcea opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/810
Core test bundles
Moved osgi test bundles to a separate project. I couldn't find a place to be happy enough with, so I ended up sticking it in utils for now, it could move later.
While the tests pass, there are a few tests in ./core that depend on these test bundles, but there is a circular dependency I won't tackle just yet. Those tests will have to move somewhere else really soon, part of the move to karaf which is coming next, reason why I kinda ignored the issue for now (no other good solution anyway).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hzbarcea/incubator-brooklyn core-test-bundles
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/810.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #810
----
commit 4de9f731595bafb751cc0f250dd1a897bd5e7736
Author: Hadrian Zbarcea <ha...@apache.org>
Date: 2015-08-10T23:52:57Z
[BROOKLYN-161] Move osgi test bundles to a separate project
commit 89324dbc1fb4acabd253a520974e5a1802757f0f
Author: Hadrian Zbarcea <ha...@apache.org>
Date: 2015-08-11T01:01:27Z
[BROOKLYN-161] Remove generated jars from source repo
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Core test bundles
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/810#issuecomment-132199426
yes, the circular dep is irritating. we want core to be able to test osgi bundles which in turn use core. i think options are:
* refactor the osgi loading support so it isn't in core (a bit bad?)
* hack maven into building these deps in a phase between src and test (hard i think?)
* build these manually, after a core `-DskipTests` build and before the build which runs test
they change so infrequently and they are noise for most people so i think the last one is the best, even if it means having binaries checked in as test resources. however while refactoring is going on this will be a problem, so i suggest we wait for refactoring to stabilise then revert this, moving them back to `core/../dependencies`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Core test bundles
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/810
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Core test bundles
Posted by hzbarcea <gi...@git.apache.org>.
GitHub user hzbarcea reopened a pull request:
https://github.com/apache/incubator-brooklyn/pull/810
Core test bundles
Moved osgi test bundles to a separate project. I couldn't find a place to be happy enough with, so I ended up sticking it in utils for now, it could move later.
While the tests pass, there are a few tests in ./core that depend on these test bundles, but there is a circular dependency I won't tackle just yet. Those tests will have to move somewhere else really soon, part of the move to karaf which is coming next, reason why I kinda ignored the issue for now (no other good solution anyway).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hzbarcea/incubator-brooklyn core-test-bundles
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/810.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #810
----
commit 4de9f731595bafb751cc0f250dd1a897bd5e7736
Author: Hadrian Zbarcea <ha...@apache.org>
Date: 2015-08-10T23:52:57Z
[BROOKLYN-161] Move osgi test bundles to a separate project
commit 89324dbc1fb4acabd253a520974e5a1802757f0f
Author: Hadrian Zbarcea <ha...@apache.org>
Date: 2015-08-11T01:01:27Z
[BROOKLYN-161] Remove generated jars from source repo
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Core test bundles
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/810#issuecomment-132854302
@ahgittin , sorry for the late reply. For the record, no objections. Once we finish the refactoring and the karaf support we'll have more info to discuss a solution. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Core test bundles
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/810#issuecomment-130070640
@hzbarcea can you say more about why you deleted the test jars here, and disabled the tests?
These jars make writing OSGi tests much simpler (because the build doesn't need to generate these artifacts from other modules first, and we don't need to worry as much about the classes accidentally being on the classpath). We never liked having the tests jars checked in, but it was worth it to get the test coverage.
Why disable the OSGi tests before the Karaf work starts?
@ahgittin any comments?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Core test bundles
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/810#issuecomment-130092400
@aledsage good question. There are a few answers, but the most relevant one is that it makes the package rename more time consuming and error prone. Last week right after the catalog rename in api, I started to work on the location and the tools were of no help because of the circular dependencies and all. There are way to many files to make adjustments manually and we want to get over this part fast.
We agree on the jars being checked in. Although I mentioned karaf, it has little to do with it, although I think we can get started on that next week after finishing the package rename. The biggest problem is actually the circular dependency in core, but there is a simple (possibly temporary) solution, to move the few tests depending on these jars outside the core (say in utils/test-core or something) and re-enable them.
In the original mail that started this work you (@aledsage) correctly stated that we should keep the tests running at all times, otherwise stabilizing the code will be a nightmare and we should err on the side of speed. This is indeed cutting a short corner.
I will merge this (I ran the tests again and they pass) just to unblock my refactoring of ./api, but if you prefer I can move and re-enable the tests, or even revert this patch if you think it's a bad idea. The alternative is to rebuild the test jars all the time, which is unpleasant in another way. It's all about choices. I don't have a particular preference but I thought this is faster and aligned with the near future.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Core test bundles
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/810#issuecomment-130112186
Merging for now, let's decide tomorrow how to follow up.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Core test bundles
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/810#issuecomment-130221855
@hzbarcea thanks; agree with you - it makes sense to disable these tests during the rename. I hadn't thought about the incremental changes requiring us to rebuild the jars for every PR. Let's just do it once at the end.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Core test bundles
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/810#issuecomment-132643745
as discussed with @hzbarcea there is the option to have a multiple git repos as part of brooklyn, so another option (a variant of the last one in my list above) would be to have test artifacts in a completely separate repo which we reference. there is overhead as they have to be pushed to maven central but it is undoubtedly cleaner.
my pragmatic short-term preference is still to push it back into `core/.../dependencies` because that worked and it has only been an issue when we did the massive refactoring. otherwise they stay hidden and we get the test coverage we need (which currently we don't have).
@hzbarcea any objections?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Core test bundles
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/810
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---