You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by lawrencecraft <gi...@git.apache.org> on 2017/11/11 22:16:04 UTC
[GitHub] storm pull request #2414: Fixing Java unit tests in storm-core to run under ...
GitHub user lawrencecraft opened a pull request:
https://github.com/apache/storm/pull/2414
Fixing Java unit tests in storm-core to run under Windows (1.x branch)
While fixing STORM-2797 I noticed a bunch of the storm-core tests didn't run under Windows.
I've fixed them up to use Windows-friendly asserts. All are done in a generic way through framework classes except one particular case where the behavior actually differs based on operating system. I only modified tests, not any actual application code. I tested this change on Windows, OS X, and Linux: all pass. On Windows, I still have to run Maven as administrator as some of the tests write to temp folders in AppData.
I'll follow up with a pull request for STORM-2797. There are still some Clojure tests failing but a lot of them are around the logviewer, which will be fixed by my next PR.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lawrencecraft/storm WindowsTestFix-1.x
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2414.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 #2414
----
commit c32af1f4386044312357fe39c0789a41539aab6a
Author: Lawrence Craft <la...@gmail.com>
Date: 2017-11-11T16:21:14Z
Fixing Java unit tests in storm-core to run under Windows
----
---
[GitHub] storm pull request #2414: Fixing Java unit tests in storm-core to run under ...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2414#discussion_r150402419
--- Diff: storm-core/test/jvm/org/apache/storm/daemon/supervisor/ContainerTest.java ---
@@ -125,7 +125,8 @@ public void testKill() throws Exception {
private static final Joiner PATH_JOIN = Joiner.on(File.separator).skipNulls();
private static final String DOUBLE_SEP = File.separator + File.separator;
static String asAbsPath(String ... parts) {
- return (File.separator + PATH_JOIN.join(parts)).replace(DOUBLE_SEP, File.separator);
+ String path = (File.separator + PATH_JOIN.join(parts)).replace(DOUBLE_SEP, File.separator);
--- End diff --
I think starting a path with `/` or `\` is inherently going to be fragile on Windows. It might be better to remove the initial file separator from here, and throw an exception if the first path part is `File.separator` . Then you can probably also get rid of the `DOUBLE_SEP` replacement. Same check should be in `asPath`. In order to work on Windows, paths should start from e.g. `java.io.tmpdir` or the current working directory instead of the filesystem root I think.
---
[GitHub] storm pull request #2414: Fixing Java unit tests in storm-core to run under ...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2414#discussion_r150402295
--- Diff: storm-core/test/jvm/org/apache/storm/localizer/AsyncLocalizerTest.java ---
@@ -62,15 +62,15 @@ public void testRequestDownloadBaseTopologyBlobs() throws Exception {
final String jarKey = topoId + "-stormjar.jar";
final String codeKey = topoId + "-stormcode.ser";
final String confKey = topoId + "-stormconf.ser";
- final String stormLocal = "/tmp/storm-local/";
- final String stormRoot = stormLocal+topoId+"/";
+ final String stormLocalAbsolutePath = new File("/tmp/storm-local/").getAbsolutePath();
--- End diff --
Use `Files.createTempFile` instead of `/tmp`.
---
[GitHub] storm issue #2414: Fixing Java unit tests in storm-core to run under Windows...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2414
Sounds good.
---
[GitHub] storm issue #2414: Fixing Java unit tests in storm-core to run under Windows...
Posted by lawrencecraft <gi...@git.apache.org>.
Github user lawrencecraft commented on the issue:
https://github.com/apache/storm/pull/2414
Thanks Stig, will do.
I might submit a fix for STORM-2797 (the LogViewer issue) first, if that's alright, as I've got that almost ready to go. I can track the fixing of the unit tests separately.
---
[GitHub] storm pull request #2414: Fixing Java unit tests in storm-core to run under ...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2414#discussion_r150402445
--- Diff: storm-core/test/jvm/org/apache/storm/daemon/supervisor/BasicContainerTest.java ---
@@ -370,7 +370,7 @@ public void testLaunch() throws Exception {
final String workerConf = ContainerTest.asAbsPath(log4jdir, "worker.xml");
--- End diff --
If you start the paths from `Files.createTempFile` instead of ``tmp` for `stormHome` and `stormLocal`, you should be able to get rid of the `prependFilePrefixIfOnWindows` method.
---