Bugzilla – Full Text Bug Listing
| Summary: | Move org.eclipse.ecf.remoteservice.rest to httpclient 4 (from httpclient 3.1) | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [RT] ECF | Reporter: | Scott Lewis <slewis> | ||||
| Component: | ecf.remoteservices | Assignee: | ecf.core-inbox <ecf.core-inbox> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | enhancement | ||||||
| Priority: | P3 | CC: | bugs.eclipse.org, sebastian.schmidt2 | ||||
| Version: | unspecified | Keywords: | helpwanted | ||||
| Target Milestone: | 3.6.1 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows 8 | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Scott Lewis
I gave this a first shot and committed a patch to move from 3.1 to 4.1.2 to my github fork of ECF: https://github.com/sschmidt/ecf/commit/7534a3c111341223a799903a724964bd3ef39c1e (On a side note: do we have a better way to contribute patches for testing. Maybe access to the foundation's gerrit server?) Implementation was pretty straightforward. The tests in o.e.e.test.remoteservice.rest are green with the new implementation. The one issue I had is that httpclient 4.x doesn't seem to support parameters for get requests (see prepareGetMethod in RestClientService). We supported this using 3.1. Then again, I'm not sure if it's even supported by the http specification. Please let me know if someone finds a workaround. What would be the best way to test the new implementation before committing it? (In reply to comment #1) > (On a side note: do we have a better way to contribute patches for testing. > Maybe access to the foundation's gerrit server?) What's wrong with a Github pull request? Or do you want Jenkins to run the tests for you triggered by e.g. Gerrit? Btw #386286 asks for ECF gerrit activation. (In reply to comment #1) > I gave this a first shot and committed a patch to move from 3.1 to 4.1.2 to > my github fork of ECF: > https://github.com/sschmidt/ecf/commit/ > 7534a3c111341223a799903a724964bd3ef39c1e > (On a side note: do we have a better way to contribute patches for testing. > Maybe access to the foundation's gerrit server?) That would be fine, but I believe it's going to take some setup for ECF...and that hasn't been done yet. http://wiki.eclipse.org/Gerrit So in the mean time, please attach to this bug as a patch (that allows ip review flag to be set). > > Implementation was pretty straightforward. The tests in > o.e.e.test.remoteservice.rest are green with the new implementation. > > The one issue I had is that httpclient 4.x doesn't seem to support > parameters for get requests (see prepareGetMethod in RestClientService). We > supported this using 3.1. Then again, I'm not sure if it's even supported by > the http specification. Please let me know if someone finds a workaround. > > What would be the best way to test the new implementation before committing > it? As for testing...some rest service either has to be created (e.g. on jetty) or an existing/public service identified. We can use the machine at OSU OSL for server testing...if that's needed. Created attachment 228834 [details]
Patch (version 1)
(In reply to comment #3) > As for testing...some rest service either has to be created (e.g. on jetty) > or an existing/public service identified. We can use the machine at OSU > OSL for server testing...if that's needed. I attached the patch. There seem to be some tests that call Twitter API. However, they're not in the test set invoked by the supplied launch configuration. I'll try to activate them and see if errors occur. However, if some adopters could test the new implementation in different scenarios too, this would really help. Do you want to replace the current implementation by the httpclient 4.x implementation or go for a similar approach as the filetransfer provider (i.e. maintain a httpclient and httpclient 4.x version in parallel) for some time? Hi Sebastian, Thanks for attaching the patch. (In reply to comment #5) > (In reply to comment #3) > > As for testing...some rest service either has to be created (e.g. on jetty) > > or an existing/public service identified. We can use the machine at OSU > > OSL for server testing...if that's needed. > > I attached the patch. There seem to be some tests that call Twitter API. > However, they're not in the test set invoked by the supplied launch > configuration. The main reason they are not in the test set is that Twitter changed it's API (authentication), and the tests haven't been updated appropriately. >I'll try to activate them and see if errors occur. Unfortunately, I don't think they will work. > However, if some adopters could test the new implementation in different > scenarios too, this would really help. > Do you want to replace the current implementation by the httpclient 4.x > implementation or go for a similar approach as the filetransfer provider > (i.e. maintain a httpclient and httpclient 4.x version in parallel) for some > time? I would prefer to have the 4.x implementation replace the current implementation. (In reply to comment #6) > I would prefer to have the 4.x implementation replace the current > implementation. Ok, the current patch does exactly that. I'll try to update the Twitter-tests in order to have some real-word code to test the new implementation. IMHO it's a good thing that the new implementation is conform to all currently active tests. Then again, the tests doesn't seem to cover all scenarios in which people use our REST service. Again, if adopters could test it with their respective code, this would help a lot. (In reply to comment #7) > (In reply to comment #6) > > I would prefer to have the 4.x implementation replace the current > > implementation. > > Ok, the current patch does exactly that. > > I'll try to update the Twitter-tests in order to have some real-word code to > test the new implementation. Ok. You might want to correspond with committer Wim Jongman about updating the twitter tests...as I think he did some work on it...or maybe just looked at it...I'm not sure. > IMHO it's a good thing that the new implementation is conform to all > currently active tests. Then again, the tests doesn't seem to cover all > scenarios in which people use our REST service. No...we need more tests. The really difficult thing about the remoteservice tests (rest API but not just rest API) is having a test environment that has associated services running, etc...in other words a test environment. Markus and Jasintha (I think) have done some work in this area (remote services testing), and maybe we can take some advantage of that work...but I'm not sure where it is now. > Again, if adopters could test it with their respective code, this would help > a lot. Yes. Short of a new release, however...I'm not sure how best to enable such testing. I created a gerrit change request for this bug: https://git.eclipse.org/r/#/c/11942/ Not sure if gerrit sends out notification, thus cross-posting here: Hi Sebastian, you haven't change the httpclient imports for org.eclipse.ecf.tests.remoteservice.rest. Thanks Markus (In reply to comment #10) > Not sure if gerrit sends out notification, thus cross-posting here: > > Hi Sebastian, > > you haven't change the httpclient imports for > org.eclipse.ecf.tests.remoteservice.rest. > > Thanks > Markus Hi Sebastian, Markus and others I would *like* to include this fix in ECF's contribution to Kepler...i.e. ECF 3.6.1. That would mean, however, getting the test code changed over, this whole thing reviewed and tested, and fully in the build by the end of May. Is this going to be doable? I'm not sure exactly where this is...it looks from the comments like there are a little more changes to test code needed, and then a little review and testing. Do all involved think this can happen quickly? I will contribute review and testing resources over the next couple of weeks, if that's needed. Thanks...please inform on this bug. For the moment, I'll set the target to 3.6.1. But if this can't/doesn't work, then please feel free to change target. Thanks for your contributions. Marking as fixed as per http://git.eclipse.org/c/ecf/org.eclipse.ecf.git/commit/?id=4236968a3d7aac3abf0ad58f80e2628f400b352e Thanks Sebastian |