You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Andrew Phillips <no...@github.com> on 2014/05/31 07:20:10 UTC
[jclouds] Adding a test for 278 (#389)
Contributed by Bill Branan. Looks like this succeeds on master _without_ any code changes (see #388)?
CC @bbranan @andrewgaul
You can merge this Pull Request by running:
git pull https://github.com/jclouds/jclouds test-for-jclouds-278
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/389
-- Commit Summary --
* Adding a test for 278
-- File Changes --
A apis/swift/src/test/java/org/jclouds/openstack/swift/functions/ParseObjectInfoListFromJsonResponseTest.java (73)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/389.patch
https://github.com/jclouds/jclouds/pull/389.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389
Re: [jclouds] Adding a test for 278 (#389)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1179](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1179/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-44762282
Re: [jclouds] Adding a test for 278 (#389)
Posted by Andrew Phillips <no...@github.com>.
@andrewgaul Is this good to merge for you (pending a squash'n'rebase, obviously)?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-44836914
Re: [jclouds] Adding a test for 278 (#389)
Posted by Andrew Phillips <no...@github.com>.
> +import java.io.InputStream;
> +import java.util.ArrayList;
> +import java.util.List;
> +
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertNotNull;
> +
> +/**
> + * @author Bill Branan
> + */
> +@Test(groups = "unit")
> +public class ParseObjectInfoListFromJsonResponseTest {
> +
> + @Test
> + public void testNoUrlDecodingOfResponse() throws Exception {
> + String contentName = "swift-test-content-%20-1401395399020";
@bbranan changes `%x` to `%20` to make it match an actual URL encoded sequence, but this test _still_ passes for me locally. Let's see what the PR builders say.
What is the character sequence that was actually causing the failures for you?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389/files#r13259440
Re: [jclouds] Adding a test for 278 (#389)
Posted by Andrew Phillips <no...@github.com>.
@andrewgaul: Any chance of a quick review here? Thanks! ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-45130911
Re: [jclouds] Adding a test for 278 (#389)
Posted by Andrew Gaul <no...@github.com>.
@demobox Sorry for the delay, this looks good!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-46982017
Re: [jclouds] Adding a test for 278 (#389)
Posted by Bill Branan <no...@github.com>.
It's good to hear that this issue was resolved on master prior to my changes. Including the test in this pull request seems like a good way to verify that the issue remains resolved. Thanks for your help in getting this one sorted out.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-44835916
Re: [jclouds] Adding a test for 278 (#389)
Posted by Bill Branan <no...@github.com>.
@demobox The code on master does work, which is great. I'll need to wait for a release version before I can pull it into my codebase. Any idea when that might happen?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-44841835
Re: [jclouds] Adding a test for 278 (#389)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1323](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1323/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-44761335
Re: [jclouds] Adding a test for 278 (#389)
Posted by Andrew Phillips <no...@github.com>.
Closed #389.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#event-129482204
Re: [jclouds] Adding a test for 278 (#389)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1177](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1177/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-44719821
Re: [jclouds] Adding a test for 278 (#389)
Posted by Andrew Phillips <no...@github.com>.
> Including the test in this pull request seems like a good way to verify that the issue remains resolved.
Planning to merge that soon. Is master something you'd be able to use, @bbranan? Otherwise, would you be able to see if backporting 12f29fd to the branch you're using helps?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-44836773
Re: [jclouds] Adding a test for 278 (#389)
Posted by Andrew Phillips <no...@github.com>.
@bbranan: Even with `swift-test-content-%x-1401395399020`, the test seems to [succeed on master](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/org.apache.jclouds.api$swift/1323/testReport/org.jclouds.openstack.swift.functions/ParseObjectInfoListFromJsonResponseTest/testNoUrlDecodingOfResponse/) without code changes.
@andrewgaul: Since 12f29fd seems to fix [278](https://issues.apache.org/jira/browse/JCLOUDS-278), worth backporting that?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-44761426
Re: [jclouds] Adding a test for 278 (#389)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #851](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/851/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-44719304
Re: [jclouds] Adding a test for 278 (#389)
Posted by Andrew Phillips <no...@github.com>.
> jclouds-pull-requests #852 FAILURE
Some Jenkins problem?
```
GitHub pull request #389 to jclouds/jclouds
FATAL: null
java.lang.NullPointerException
````
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-44760016
Re: [jclouds] Adding a test for 278 (#389)
Posted by Andrew Phillips <no...@github.com>.
Whoops! Bunch of Checkstyle violations in this file. Will clean them up asap...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-45564866
Re: [jclouds] Adding a test for 278 (#389)
Posted by Andrew Phillips <no...@github.com>.
> Bunch of Checkstyle violations in this PR. Will clean them up asap...
See https://github.com/jclouds/jclouds/pull/400
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-45565496
Re: [jclouds] Adding a test for 278 (#389)
Posted by Andrew Phillips <no...@github.com>.
> I actually used %x intentionally because it is not a valid URL encoding.
Ah, OK. I'll update the test and try again. Thanks for explaining, @bbranan!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-44759405
Re: [jclouds] Adding a test for 278 (#389)
Posted by Andrew Phillips <no...@github.com>.
Merged to [master](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;a=commit;h=b1c163636dbf07b360f85f2ca8a3e570e9c494bd).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-45532104
Re: [jclouds] Adding a test for 278 (#389)
Posted by Bill Branan <no...@github.com>.
> +import java.io.InputStream;
> +import java.util.ArrayList;
> +import java.util.List;
> +
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertNotNull;
> +
> +/**
> + * @author Bill Branan
> + */
> +@Test(groups = "unit")
> +public class ParseObjectInfoListFromJsonResponseTest {
> +
> + @Test
> + public void testNoUrlDecodingOfResponse() throws Exception {
> + String contentName = "swift-test-content-%20-1401395399020";
I actually used %x intentionally because it is not a valid URL encoding. The error I was trying to correct for is when the content name includes a % character that is not a valid URL encoding. For example: "graph-of-75%-closure.png" or "report-50%x25%-success.xls". These types of names show up in the content sets of my customers, and trying to get a list of content items fails without my patch. Making the change to %20 here removes the purpose of the test.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389/files#r13261156
Re: [jclouds] Adding a test for 278 (#389)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1322](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1322/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-44719375
Re: [jclouds] Adding a test for 278 (#389)
Posted by Andrew Phillips <no...@github.com>.
> I'll need to wait for a release version before I can pull it into my codebase.
@andrewgaul: Should we look to backport 12f29fd to 1.7.x, or is that too much of a breaking change?
@bbranan: I don't have a concrete date for 1.8.0 in my head, but it will be a couple of months, most likely.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-44846110
Re: [jclouds] Adding a test for 278 (#389)
Posted by Andrew Phillips <no...@github.com>.
> Sorry for the delay, this looks good!
No problem...thanks, @andrewgaul!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-46995685
Re: [jclouds] Adding a test for 278 (#389)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #852](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/852/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/389#issuecomment-44759709