Community
Participate
Working Groups
The this fails when run in a different time zone: http://ci.mylyn.org/view/Snapshots/job/mylyn-reviews-snapshot/383/testReport/org.eclipse.mylyn.internal.gerrit.core.remote/GerritRemoteFactoryTest/testReviewFactory/ I doubt that it's useful to assert the exact date in the test case since it depends on parts of the test data that changes when the test repository is re-provisioned.
Yeah, eek. Not sure why it just started failing, but getting rid of hour check isn't enough. OTOH, I need to make sure that we have the right date. Is checking year (presuming that we're not on Dec 31 or Jan 1!) and minute enough do you think? re: reprovisioning, all of the tests are gong to fail if we have an empty test repository, and I'm not sure that there is a way to provision a new repository consistently...?
(In reply to comment #1) > Yeah, eek. Not sure why it just started failing, but getting rid of hour check > isn't enough. OTOH, I need to make sure that we have the right date. Is checking > year (presuming that we're not on Dec 31 or Jan 1!) and minute enough do you > think? As I said, I don't believe that it makes sense to assert on the date. Generally, if you are comparing dates you need to take timezones into account, e.g. normalize the date to UTC before comparing. > re: reprovisioning, all of the tests are gong to fail if we have an empty test > repository, and I'm not sure that there is a way to provision a new repository > consistently...? Yes there is but we haven't done the work. We are not planning to support specific creation dates for reviews though.
(In reply to comment #2) > (In reply to comment #1) > > Yeah, eek. Not sure why it just started failing, but getting rid of hour check > > isn't enough. OTOH, I need to make sure that we have the right date. Is > checking > > year (presuming that we're not on Dec 31 or Jan 1!) and minute enough do you > > think? > > As I said, I don't believe that it makes sense to assert on the date. Generally, > if you are comparing dates you need to take timezones into account, e.g. > normalize the date to UTC before comparing. All that I'm concerned with is that we are actually getting a date into the model that matches what is on the back end. But I don't really care what the date itself is. > > re: reprovisioning, all of the tests are gong to fail if we have an empty test > > repository, and I'm not sure that there is a way to provision a new repository > > consistently...? > > Yes there is but we haven't done the work. We are not planning to support > specific creation dates for reviews though. Right, but I need to be able to test that we're getting something sensible there, I can't simply ignore this field. So not sure what the long-term answer is, but for now I'll just base on normalized date. Perhaps we can make this a configuration property somewhere.
(In reply to comment #3) > All that I'm concerned with is that we are actually getting a date into the > model that matches what is on the back end. But I don't really care what the > date itself is. We'll have to be pragmatic here. If you can find a way to provision a review with the specific dates and assert against that, great. Otherwise you could create a new review and then check if the date is within a reasonable delta to the current time. > > Yes there is but we haven't done the work. We are not planning to support > > specific creation dates for reviews though. > > Right, but I need to be able to test that we're getting something sensible > there, I can't simply ignore this field. So not sure what the long-term answer > is, but for now I'll just base on normalized date. Perhaps we can make this a > configuration property somewhere. No. Please don't complicate the test setup even more. Miles, I'm extending the scope of this bug. The tests also fail against Gerrit 2.5 which doesn't have the specific data that it's in the 2.4 repository. To run the AllGerritTests suite against all available repositories pass "-Dorg.eclipse.mylyn.tests.all=true" as a VM argument.
Here is a log of the failures on Hudson: https://hudson.eclipse.org/hudson/view/Mylyn/job/mylyn-integration-connectors/lastCompletedBuild/testReport/.
Thanks for the link! I guess there isn't anyway to trigger these directly, but now that I know the incantation for getting all of the versions running I should be able to test locally.
Steffen, it's not clear to me what's happening when I run this under local JUnit. I see failures, and then they're replaced with successful executions. What's going on here?
Try running as Junit 3.
(In reply to comment #8) > Try running as Junit 3. That worked, thanks.(In reply to comment #4) > (In reply to comment #3) > We'll have to be pragmatic here. If you can find a way to provision a review > with the specific dates and assert against that, great. Otherwise you could > create a new review and then check if the date is within a reasonable delta to > the current time. Good idea. The trouble here is actually discovering the Gerrit id for the latest change. Rather than do that, I'd rather zero out all of the Gerrit changes. In fact, that wouldn't be a bad idea anyway. But I don't think there is a good way to do that short of nuking the Gerrit repos for every test, and that is probably a bad idea as well. I guess I can do a query based on the latest change ID, but that adds a lot of complexity too..
(In reply to comment #9) > I guess I can do a query based on the latest change ID, but > that adds a lot of complexity too.. I don't fully understand why that's needed. When you push a new change Gerrit includes the ID of the review in the response as part of the URL. It shouldn't be terribly difficult to parse that.
(In reply to comment #10) > (In reply to comment #9) > > I guess I can do a query based on the latest change ID, but > > that adds a lot of complexity too.. > > I don't fully understand why that's needed. When you push a new change Gerrit > includes the ID of the review in the response as part of the URL. It shouldn't > be terribly difficult to parse that. The Git push returns a hash, but not the (deprecated) id, which is what the gerrit Connector uses to identify the review. It's not clear where to get that, so instead it looks like I'm may have to obtain the actual Gerrit change from GerritClient, which isn't ideal in terms of testing, but OTOH it does exercise the GerritClient more as a side-effect.
(In reply to comment #11) > The Git push returns a hash, but not the (deprecated) id, which is what the > gerrit Connector uses to identify the review. It's not clear where to get that, > so instead it looks like I'm may have to obtain the actual Gerrit change from > GerritClient, which isn't ideal in terms of testing, but OTOH it does exercise > the GerritClient more as a side-effect. That might be even easier and you can set an explicit Change-Id in the commit message in any case. The link I was referring to is part of the message.
(In reply to comment #12) > (In reply to comment #11) > > so instead it looks like I'm may have to obtain the actual Gerrit change from > > GerritClient, which isn't ideal in terms of testing, but OTOH it does exercise > > the GerritClient more as a side-effect. > > That might be even easier and you can set an explicit Change-Id in the commit > message in any case. Yeah, that's a good idea. And I'm going to need to do that anyway in order to test dependent changes. > The link I was referring to is part of the message. But it still doesn't have the change #; (I'm referring to e.g. the "10" in http://localhost:2080/gerrit-2.5.2/#/c/10/) Oh well..
Please run Gerrit ProjectTest. The output does have the ID in it: pre.. Processing changes: new: 1 Processing changes: new: 1, done New Changes: http://mylyn.org/gerrit-2.4.2/7 /tmp/gerrit8887977675731496108.tmp
Argh, you're right. I missed that. :D
Steffen, can you think of any way to improve git/gerrit performance for local box? I'm seeing about 5 secs per push. Obviously it isn't a latency issue. I've tried uppping VBox resources but that doesn't seem to help.
(In reply to comment #16) > Steffen, can you think of any way to improve git/gerrit performance for local > box? I'm seeing about 5 secs per push. Obviously it isn't a latency issue. I've > tried uppping VBox resources but that doesn't seem to help. Have you tried increasing memory for the Gerrit instance? It's set to a fairly low value so we can run multiple versions in the same VM.
(In reply to comment #17) > Have you tried increasing memory for the Gerrit instance? It's set to a fairly > low value so we can run multiple versions in the same VM. I just raised the total allocated memory for the VBox from the Oracle Tool. I'm not sure if that's the same thing you're saying..sort of ignorant about all this.
(In reply to comment #18) > I just raised the total allocated memory for the VBox from the Oracle Tool. I'm > not sure if that's the same thing you're saying..sort of ignorant about all > this. Not it's not. I was talking about the JVM settings for the Gerrit instances. You can change the settiings in the gerrit.config under /home/tools/gerrit. It's currently set to "heapLimit = 128m".
No joy. Oh well. (n.b. the config location is /home/tools/gerrit/gerrit-{version}/etc/gerrit.config)
https://git.eclipse.org/r/#/c/12210/ Steffen, can you trigger a integration build off this, or do we need to commit to master first?
We don't have it setup so that we can run integration builds against particular patch sets. Let's just commit and apply fixes in a follow up change if needed. Thanks for looking into this!
Merged, but I'll wait to close until we see the integraiton tests actually passing.
There are still failures that maybe related to dates not being adjusted to local time zone: https://hudson.eclipse.org/hudson/job/mylyn-integration-connectors/lastCompletedBuild/testReport/org.eclipse.mylyn.internal.gerrit.core.remote/GerritRemoteFactoryTest/testGlobalComments_2_5_2/ java.lang.AssertionError: Expected: (a value greater than <0L> and a value less than <7500L>) got: <-158111L> at org.junit.Assert.assertThat(Assert.java:778) at org.junit.Assert.assertThat(Assert.java:736) at org.eclipse.mylyn.internal.gerrit.core.remote.GerritRemoteFactoryTest$ReviewHarness.init(GerritRemoteFactoryTest.java:137) at org.eclipse.mylyn.internal.gerrit.core.remote.GerritRemoteFactoryTest.setUp(GerritRemoteFactoryTest.java:158)
I think that was just having the time out too short -- see bug 406693 comment 2. (Aren't the servers co-located? If not, it's an easy fix, we just need to bump up the time to day.) But we won't know until we fix what ever is going wrong in bug 406693.
(In reply to comment #25) > I think that was just having the time out too short -- see bug 406693 comment 2. > (Aren't the servers co-located? No. The repositories are located in Germany while the build server is in Canada.
Alright, I admit that I'm really confused right now, maybe it's a Friday thing. First, the test is passing now, again, I think the issue was simply that I had the timeout too low. This change fixes that..if it is still a concern after your fixes https://git.eclipse.org/r/#/c/12245/ See https://git.eclipse.org/r/#/c/12258/1/org.eclipse.mylyn.gerrit.tests/src/org/eclipse/mylyn/internal/gerrit/core/remote/GerritRemoteFactoryTest.java. The current test then simply checks System against our servers in different time zones with a brief delay. The proposed (abandoned) test leaves +- 24 hours to allow for any time zone differences. Both solutions can't be wrong, can they?
I'll run another integration build with the latest changes. It's not quite clear to me why this fails in the integration tests but not on Hudson. It could be that the clock on the slave executing integration tests is out of sync. I also kicked of a snapshot here to check how this behaves on mylyn.org: http://ci.mylyn.org/job/mylyn-reviews-snapshot/403/.
The other one's are failing and I'm really thinking it's the timeout issue. I'm restoring https://git.eclipse.org/r/#/c/12245 and let's see how that does.
(In reply to comment #29) > The other one's are failing and I'm really thinking it's the timeout issue. I'm > restoring https://git.eclipse.org/r/#/c/12245 and let's see how that does. Agreed. If I'm reading the patch right, we now allow a delta of 15 seconds. That still seems very optimistic :). We could set that to 2 minutes to allow for a bit of lag.
(In reply to comment #30) > Agreed. If I'm reading the patch right, we now allow a delta of 15 seconds. That > still seems very optimistic :). We could set that to 2 minutes to allow for a > bit of lag. Yeah, but note that we'd also have to do this for all of the timeouts under the assumption that Gerrit could take that long, which is a long time to wait, especially if you're looking at failures locally. :) In practice, I've noticed that network latency doesn't seem to be the constraint. It's seems to be mostly Gerrit processing time, because we get nearly the same lag locally. And as discussed above, it doesn't seem to matter what resources the server has. So I've just roughly doubled the typical response time. I'm going to close this now and we can reopen if we have timing issues again.
The test is still failing here: https://hudson.eclipse.org/hudson/view/Mylyn/job/mylyn-integration-connectors/137/ The build was triggered on: Apr 26, 2013 7:40:37 PM The local time on the slave was: Fri Apr 26 23:38:08 UTC 2013 Expected: (a value greater than <0L> and a value less than <7500L>) got: <-158594L> So it looks like the test time is off by 2.5 minutes? I would just allow a reasonably large time window (e.g. 30 min.) to account for clock scew but I may misinterpret the test output.
There is another problem that I can also reproduce in my workspace (using mylyn-dev-all.target): http://ci.mylyn.org/job/mylyn-3.9.x/41/TARGET=kepler/testReport/junit/org.eclipse.mylyn.internal.gerrit.core.remote/GerritRemoteFactoryTest/testGlobalComments_2_4_2/ java.lang.NoSuchMethodError: org.hamcrest.core.AnyOf.anyOf([Lorg/hamcrest/Matcher;)Lorg/hamcrest/Matcher; at org.hamcrest.number.OrderingComparisons.greaterThanOrEqualTo(OrderingComparisons.java:20) at org.hamcrest.number.OrderingComparisons.lessThan(OrderingComparisons.java:27) at org.hamcrest.Matchers.lessThan(Matchers.java:238) at org.eclipse.mylyn.internal.gerrit.core.remote.GerritRemoteFactoryTest$ReviewHarness.init(GerritRemoteFactoryTest.java:138) at org.eclipse.mylyn.internal.gerrit.core.remote.GerritRemoteFactoryTest.setUp(GerritRemoteFactoryTest.java:158)
See https://git.eclipse.org/r/#/c/12318/ for both.. (In reply to comment #32) > So it looks like the test time is off by 2.5 minutes? I would just allow a > reasonably large time window (e.g. 30 min.) to account for clock scew but I may > misinterpret the test output. No, you have it right. The above takes your suggestion, and I've seperated it out from the timeout issue. (In reply to comment #33) > java.lang.NoSuchMethodError: > org.hamcrest.core.AnyOf.anyOf([Lorg/hamcrest/Matcher;)Lorg/hamcrest/Matcher; > at > org.hamcrest.number.OrderingComparisons.greaterThanOrEqualTo(OrderingComparisons.java:20) > at Wow, weird, right? How can it be calling a non-existing method in the same library?! I thought below might be the culprit. Looks like junit includes its own version of the Matcher class. http://stackoverflow.com/questions/7869711/getting-nosuchmethoderror-org-hamcrest-matcher-describemismatch-when-running So in the review, I've also changed the required-bundle order. Not sure if that will affect activation order or not.. But one other possibility that occurred to me is that we have some kind of generics related issue because I didn't coerce both arguments to longs, so I've corrected that.
All tests passed in the latest test run: https://hudson.eclipse.org/hudson/view/Mylyn/job/mylyn-integration-connectors/140/. Thanks for hanging in there, Miles!
Wow, what a bear!
Looks like this is still failing on Kepler: http://ci.mylyn.org/job/mylyn-3.9.x/TARGET=kepler/45/testReport/junit/org.eclipse.mylyn.internal.gerrit.core.remote/GerritRemoteFactoryTest/testGlobalComments_2_4_2/ java.lang.NoSuchMethodError: org.hamcrest.core.AnyOf.anyOf([Lorg/hamcrest/Matcher;)Lorg/hamcrest/Matcher; at org.hamcrest.number.OrderingComparisons.greaterThanOrEqualTo(OrderingComparisons.java:20) at org.hamcrest.number.OrderingComparisons.lessThan(OrderingComparisons.java:27) at org.hamcrest.Matchers.lessThan(Matchers.java:238) at org.eclipse.mylyn.internal.gerrit.core.remote.GerritRemoteFactoryTest$ReviewHarness.init(GerritRemoteFactoryTest.java:141)
Note that Kepler comes with a newer version of Hamcrest so that might be part of the problem.
Argh. Feels like Night of the Living Bug! I need to focus on the persistence stuff right now, but I guess we need to resolve this for M7. I may give up and just change that assert not to use the hamcrest comparison logic. I'm sure there is some fascinating dependency issue but it's prbably not worht digging into much more deeply than this..
It sounds like we are hitting bug 403676. The 1.3 version of hamcrest that Kepler depends on has binary incompatible changes. I played around with version constraints but I couldn't get PDE build to resolve the right version so it would be best to not use the AnyOf.anyOf() APIs.
Merged: https://git.eclipse.org/r/#/c/12465/ Let's see if this one does it!
Thanks Miles. That worked but one spot was missed: http://ci.mylyn.org/job/mylyn-3.9.x/49/TARGET=kepler/testReport/junit/org.eclipse.mylyn.internal.gerrit.core.remote/GerritRemoteFactoryTest/testNewChange_2_4_2/ org.hamcrest.core.AnyOf.anyOf([Lorg/hamcrest/Matcher;)Lorg/hamcrest/Matcher; Stacktrace java.lang.NoSuchMethodError: org.hamcrest.core.AnyOf.anyOf([Lorg/hamcrest/Matcher;)Lorg/hamcrest/Matcher;
Looks like this is done now with the fix for https://git.eclipse.org/r/#/c/12497/. Thanks!
