You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by vectorijk <gi...@git.apache.org> on 2016/12/09 11:15:48 UTC
[GitHub] geode pull request #308: [GEODE-2167] BundledJarsJUnitTest.verifyBundledJars...
GitHub user vectorijk opened a pull request:
https://github.com/apache/geode/pull/308
[GEODE-2167] BundledJarsJUnitTest.verifyBundledJarsHaveNotChanged fails with java.nio.file.InvalidPathException on Windows
error message:
```java.nio.file.InvalidPathException: Illegal char <:> at index 2: /C:/dev/gemfire/open/geode-assembly/build/resources/test/expected_jars.txt```
expectedJarFile path should be set on Windows properly. We should trim path string to elimination first '/' letter.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vectorijk/geode GEODE-2167
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode/pull/308.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 #308
----
commit 7519364fae18b3ba63c4347d1e1f0734b283f4b9
Author: Kai Jiang <ji...@gmail.com>
Date: 2016-12-09T09:32:11Z
BundledJarsJUnitTest.verifyBundledJarsHaveNotChanged fails on Windows
expectedJarFile path should be set on Windows properly.
----
---
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] geode issue #308: [GEODE-2167] BundledJarsJUnitTest fails on Windows
Posted by vectorijk <gi...@git.apache.org>.
Github user vectorijk commented on the issue:
https://github.com/apache/geode/pull/308
cc @kirklund
---
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] geode pull request #308: [GEODE-2167] BundledJarsJUnitTest fails on Windows
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/308#discussion_r93131489
--- Diff: geode-core/src/test/java/org/apache/geode/util/test/TestUtil.java ---
@@ -46,11 +47,15 @@ public static String getResourcePath(Class<?> clazz, String name) {
File tmpFile = File.createTempFile(filename, null);
tmpFile.deleteOnExit();
FileUtil.copy(resource, tmpFile);
- return tmpFile.getAbsolutePath();
+ return compatibleWithWindows(tmpFile.getAbsolutePath());
}
- return path;
+ return compatibleWithWindows(path);
} catch (URISyntaxException | IOException e) {
throw new RuntimeException("Failed getting path to resource " + name, e);
}
}
+
+ private static String compatibleWithWindows(String path) {
--- End diff --
maybe make it public and call it getPath(File file)?
---
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] geode pull request #308: [GEODE-2167] BundledJarsJUnitTest fails on Windows
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/308#discussion_r91988138
--- Diff: geode-assembly/src/test/java/org/apache/geode/BundledJarsJUnitTest.java ---
@@ -48,8 +49,10 @@
public void loadExpectedJars() throws IOException {
String expectedJarFile =
--- End diff --
Would it be better to move the check into TestUtil.getResourcePath? so that the caller do not have to worry about the platform?
---
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] geode pull request #308: [GEODE-2167] BundledJarsJUnitTest fails on Windows
Posted by kirklund <gi...@git.apache.org>.
Github user kirklund commented on a diff in the pull request:
https://github.com/apache/geode/pull/308#discussion_r93148464
--- Diff: geode-core/src/test/java/org/apache/geode/util/test/TestUtil.java ---
@@ -46,11 +47,15 @@ public static String getResourcePath(Class<?> clazz, String name) {
File tmpFile = File.createTempFile(filename, null);
tmpFile.deleteOnExit();
FileUtil.copy(resource, tmpFile);
- return tmpFile.getAbsolutePath();
+ return compatibleWithWindows(tmpFile.getAbsolutePath());
}
- return path;
+ return compatibleWithWindows(path);
} catch (URISyntaxException | IOException e) {
throw new RuntimeException("Failed getting path to resource " + name, e);
}
}
+
+ private static String compatibleWithWindows(String path) {
--- End diff --
I'd prefer to see it private to minimize tests using this util class. Util classes are the bane of well-organized tests. You end up with 30 tests using the same util class and by then it's like a whole product of its own and the tests don't even look like they're testing the product code anymore.
My advice: keep this as private, move the other method into the test class and make it private too.
---
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] geode issue #308: [GEODE-2167] BundledJarsJUnitTest fails on Windows
Posted by vectorijk <gi...@git.apache.org>.
Github user vectorijk commented on the issue:
https://github.com/apache/geode/pull/308
Addressed 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] geode pull request #308: [GEODE-2167] BundledJarsJUnitTest fails on Windows
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/308#discussion_r93151987
--- Diff: geode-core/src/test/java/org/apache/geode/util/test/TestUtil.java ---
@@ -46,11 +47,15 @@ public static String getResourcePath(Class<?> clazz, String name) {
File tmpFile = File.createTempFile(filename, null);
tmpFile.deleteOnExit();
FileUtil.copy(resource, tmpFile);
- return tmpFile.getAbsolutePath();
+ return compatibleWithWindows(tmpFile.getAbsolutePath());
}
- return path;
+ return compatibleWithWindows(path);
} catch (URISyntaxException | IOException e) {
throw new RuntimeException("Failed getting path to resource " + name, e);
}
}
+
+ private static String compatibleWithWindows(String path) {
--- End diff --
ok, I don't have strong preference either way.
---
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] geode issue #308: [GEODE-2167] BundledJarsJUnitTest fails on Windows
Posted by vectorijk <gi...@git.apache.org>.
Github user vectorijk commented on the issue:
https://github.com/apache/geode/pull/308
Thanks for the review @jinmeiliao @kirklund @metatype
---
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] geode pull request #308: [GEODE-2167] BundledJarsJUnitTest fails on Windows
Posted by vectorijk <gi...@git.apache.org>.
Github user vectorijk commented on a diff in the pull request:
https://github.com/apache/geode/pull/308#discussion_r92757837
--- Diff: geode-assembly/src/test/java/org/apache/geode/BundledJarsJUnitTest.java ---
@@ -48,8 +49,10 @@
public void loadExpectedJars() throws IOException {
String expectedJarFile =
--- End diff --
Cool. I will update this.
---
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] geode issue #308: [GEODE-2167] BundledJarsJUnitTest fails on Windows
Posted by kirklund <gi...@git.apache.org>.
Github user kirklund commented on the issue:
https://github.com/apache/geode/pull/308
Changes look good to me. After Jinmei's feedback is considered, I'll add my vote to merge this in. 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] geode pull request #308: [GEODE-2167] BundledJarsJUnitTest fails on Windows
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/geode/pull/308
---
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.
---