Skip to content

Fixing Java unit tests in storm-core to run under Windows (1.x branch)#2414

Closed
lawrencecraft wants to merge 1 commit into
apache:1.x-branchfrom
lawrencecraft:WindowsTestFix-1.x
Closed

Fixing Java unit tests in storm-core to run under Windows (1.x branch)#2414
lawrencecraft wants to merge 1 commit into
apache:1.x-branchfrom
lawrencecraft:WindowsTestFix-1.x

Conversation

@lawrencecraft

@lawrencecraft lawrencecraft commented Nov 11, 2017

Copy link
Copy Markdown
Contributor

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.

srdo
srdo requested changes Nov 12, 2017
View reviewed changes

@srdo srdo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lawrencecraft Thanks for the contribution. I agree that we should change the tests to be Windows compatible, left some suggestions.

Please open a new issue on jira to track this, and open a PR against master as well. We need all fixes to go into master first. This is so we can be sure not to "lose" fixes in later releases.

Comment thread storm-core/test/jvm/org/apache/storm/localizer/AsyncLocalizerTest.java
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();

@srdo srdo Nov 12, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Files.createTempFile instead of /tmp.

Comment thread storm-core/test/jvm/org/apache/storm/daemon/supervisor/ContainerTest.java
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);

@srdo srdo Nov 12, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread 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");

@srdo srdo Nov 12, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lawrencecraft

lawrencecraft commented Nov 12, 2017

Copy link
Copy Markdown
Contributor Author

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.

@srdo

srdo commented Nov 13, 2017

Copy link
Copy Markdown
Contributor

Sounds good.

d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
Closes Pull Requests unchanged in 2018
fcda81c 
We are closing stale Pull Requests to make the list more manageable.

Please re-open any Pull Request that has been closed in error.

Closes apache#608
Closes apache#639
Closes apache#640
Closes apache#648
Closes apache#662
Closes apache#668
Closes apache#692
Closes apache#705
Closes apache#724
Closes apache#728
Closes apache#730
Closes apache#753
Closes apache#803
Closes apache#854
Closes apache#922
Closes apache#986
Closes apache#992
Closes apache#1019
Closes apache#1040
Closes apache#1041
Closes apache#1043
Closes apache#1046
Closes apache#1051
Closes apache#1078
Closes apache#1146
Closes apache#1164
Closes apache#1165
Closes apache#1178
Closes apache#1213
Closes apache#1225
Closes apache#1258
Closes apache#1259
Closes apache#1268
Closes apache#1272
Closes apache#1277
Closes apache#1278
Closes apache#1288
Closes apache#1296
Closes apache#1328
Closes apache#1342
Closes apache#1353
Closes apache#1370
Closes apache#1376
Closes apache#1391
Closes apache#1395
Closes apache#1399
Closes apache#1406
Closes apache#1410
Closes apache#1422
Closes apache#1427
Closes apache#1443
Closes apache#1462
Closes apache#1468
Closes apache#1483
Closes apache#1506
Closes apache#1509
Closes apache#1515
Closes apache#1520
Closes apache#1521
Closes apache#1525
Closes apache#1527
Closes apache#1544
Closes apache#1550
Closes apache#1566
Closes apache#1569
Closes apache#1570
Closes apache#1575
Closes apache#1580
Closes apache#1584
Closes apache#1591
Closes apache#1600
Closes apache#1611
Closes apache#1613
Closes apache#1639
Closes apache#1703
Closes apache#1711
Closes apache#1719
Closes apache#1737
Closes apache#1760
Closes apache#1767
Closes apache#1768
Closes apache#1785
Closes apache#1799
Closes apache#1822
Closes apache#1824
Closes apache#1844
Closes apache#1874
Closes apache#1918
Closes apache#1928
Closes apache#1937
Closes apache#1942
Closes apache#1951
Closes apache#1957
Closes apache#1963
Closes apache#1964
Closes apache#1965
Closes apache#1967
Closes apache#1968
Closes apache#1971
Closes apache#1985
Closes apache#1986
Closes apache#1998
Closes apache#2031
Closes apache#2032
Closes apache#2071
Closes apache#2076
Closes apache#2108
Closes apache#2119
Closes apache#2128
Closes apache#2142
Closes apache#2174
Closes apache#2206
Closes apache#2297
Closes apache#2322
Closes apache#2332
Closes apache#2341
Closes apache#2377
Closes apache#2414
Closes apache#2469
d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
Closes Pull Requests unchanged in 2018
fdb95d0 
We are closing stale Pull Requests to make the list more manageable.

Please re-open any Pull Request that has been closed in error.

Closes apache#608
Closes apache#639
Closes apache#640
Closes apache#648
Closes apache#662
Closes apache#668
Closes apache#692
Closes apache#705
Closes apache#724
Closes apache#728
Closes apache#730
Closes apache#753
Closes apache#803
Closes apache#854
Closes apache#922
Closes apache#986
Closes apache#992
Closes apache#1019
Closes apache#1040
Closes apache#1041
Closes apache#1043
Closes apache#1046
Closes apache#1051
Closes apache#1078
Closes apache#1146
Closes apache#1164
Closes apache#1165
Closes apache#1178
Closes apache#1213
Closes apache#1225
Closes apache#1258
Closes apache#1259
Closes apache#1268
Closes apache#1272
Closes apache#1277
Closes apache#1278
Closes apache#1288
Closes apache#1296
Closes apache#1328
Closes apache#1342
Closes apache#1353
Closes apache#1370
Closes apache#1376
Closes apache#1391
Closes apache#1395
Closes apache#1399
Closes apache#1406
Closes apache#1410
Closes apache#1422
Closes apache#1427
Closes apache#1443
Closes apache#1462
Closes apache#1468
Closes apache#1483
Closes apache#1506
Closes apache#1509
Closes apache#1515
Closes apache#1520
Closes apache#1521
Closes apache#1525
Closes apache#1527
Closes apache#1544
Closes apache#1550
Closes apache#1566
Closes apache#1569
Closes apache#1570
Closes apache#1575
Closes apache#1580
Closes apache#1584
Closes apache#1591
Closes apache#1600
Closes apache#1611
Closes apache#1613
Closes apache#1639
Closes apache#1703
Closes apache#1711
Closes apache#1719
Closes apache#1737
Closes apache#1760
Closes apache#1767
Closes apache#1768
Closes apache#1785
Closes apache#1799
Closes apache#1822
Closes apache#1824
Closes apache#1844
Closes apache#1874
Closes apache#1918
Closes apache#1928
Closes apache#1937
Closes apache#1942
Closes apache#1951
Closes apache#1957
Closes apache#1963
Closes apache#1964
Closes apache#1965
Closes apache#1967
Closes apache#1968
Closes apache#1971
Closes apache#1985
Closes apache#1986
Closes apache#1998
Closes apache#2031
Closes apache#2032
Closes apache#2071
Closes apache#2076
Closes apache#2108
Closes apache#2119
Closes apache#2128
Closes apache#2142
Closes apache#2174
Closes apache#2206
Closes apache#2297
Closes apache#2322
Closes apache#2332
Closes apache#2341
Closes apache#2377
Closes apache#2414
Closes apache#2469
@d2r d2r mentioned this pull request Oct 16, 2018
Merged
@asfgit asfgit closed this in #2880 Oct 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srdo srdo srdo requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lawrencecraft @srdo