<< Back to previous view

[CLJ-1076] pprint tests fail on Windows, expecting \n Created: 26/Sep/12  Updated: 25/Oct/13  Resolved: 25/Oct/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Critical
Reporter: Ivan Kozik Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: print

Windows 7

Attachments: Text File clj-1076-fix-tests-on-windows-patch-v1.txt     Text File clj-1076-fix-tests-on-windows-patch-v2.txt     Text File clj-1076-v3.txt     Text File pprint_test_failures_01b4cb7156.txt    
Patch: Code and Test
Approval: Ok


New pprint tests were committed recently, but they fail on Windows because the tests check for \n, while pprint seems to output \r\n. A log with the test failures is attached.

The first failing commit is https://github.com/clojure/clojure/commit/4ca0f7ea17888ba7ed56d2fde0bc2d6397e8e1c0

Patch: clj-1076-v3.txt

Approach: Before comparing output of pprint against a string in the unit test, split each of those strings into sequences of lines using clojure.string/split-lines, which removes occurrences of the regex #"\r?\n" between lines, and can thus safely be used to compare multiline strings between platforms that use only a newline, and those that use carriage return plus newline.

Screened by: Alex Miller

Comment by Andy Fingerhut [ 29/Sep/12 2:27 PM ]

Patch clj-1076-fix-tests-on-windows-patch-v1.txt dated Sep 29 2012 when applied to the particular commit that Ivan mentions causes the tests to pass when I run "ant" on a Windows 7 machine for me, and it continues to pass all tests on Mac OS X 10.6.8, too.

I may be doing something wrong, but when I try to run "ant" to build and test on Windows 7 with latest Clojure master, with or without this patch, it fails right at the beginning of the tests because it can't find clojure.test.generative. I'm probably doing something wrong somewhere. Ivan, would you be able to test this patch on Windows with the latest Clojure master to see if all tests pass for you now?

Comment by Ivan Kozik [ 29/Sep/12 2:59 PM ]

All tests pass on Windows 7 here with the patch.

Ant can't find my test.generative either because it isn't in my "${maven.test.classpath}". I put it in CLASSPATH and modified my build.xml like this:

<java classname="clojure.main" failonerror="false" fork="true">
+ <pathelement path="${java.class.path}"/>
<pathelement path="${maven.test.classpath}"/>

Comment by Andy Fingerhut [ 10/Dec/12 1:33 PM ]

Just as a rough idea of how often people are hitting this issue, CLJ-1123 was opened on Dec 9 2012 and then closed when the ticket creator realized it was a duplicate of this one.

Comment by Mike Anderson [ 18/Jan/13 7:44 PM ]

Hi there is this likely to get fixed soon?

I'd like to help contribute some more patches to Clojure but it's tricky to do when I can't get the build to work

Comment by Andy Fingerhut [ 18/Jan/13 8:39 PM ]

I do not know if or when this patch will be committed to Clojure.

I can tell you that you can apply the patch to your own local copy of the Clojure source code, and then develop new Clojure patches based upon that version. The patch that fixes this problem only affects one test file, so it is unlikely to conflict with any changes you develop and submit.

Comment by Mike Anderson [ 21/Jan/13 6:36 AM ]

I can confirm this patch works fine for me on Windows with Maven/Eclipse

Suggest this patch gets pushed through approval and applied ASAP? It's a pretty obvious fix that is breaking the build....

Comment by Stuart Halloway [ 01/Mar/13 12:44 PM ]

This patch is sloppy – it makes unnecessary whitespace changes in several places.

Would it be better to make the tests trailing whitespace agnostic? Otherwise this feels like poking and prodding until the build box is happy.

Comment by Andy Fingerhut [ 02/Mar/13 2:50 PM ]

Patch clj-1076-fix-tests-on-windows-patch-v2.txt dated Mar 2, 2013 fixes pprint tests on Windows in a different way: Removing all occurrences of carriage return (\r) characters in the output of pprint before comparing it to the expected string.

I tried simply doing str/trim-newline to remove newlines and carriage returns at the end of the string, but that does not make the tests pass. They still fail due to carriage returns in the middle of the string.

Comment by Andy Fingerhut [ 02/Mar/13 2:51 PM ]

Presumptuously changing Approval from Incomplete back to None, since there is a new patch attached that should address the reason it was marked Incomplete.

Comment by Alex Miller [ 02/Sep/13 7:37 PM ]

Is this still happening on Windows?

Comment by Andy Fingerhut [ 02/Sep/13 8:27 PM ]

Yes, the latest version of Clojure still fails tests for the same reason as reported in this ticket, and both of the patches still allow the tests to pass on Windows as well as Linux and OS X.

I have thought of yet another way to allow the tests to pass on all of these platforms, which is to call clojure.string/split-lines on the multi-line strings to be compared, and then compare the resulting sequences of single-line strings for equality. That is just a slightly different way of eliminating the line terminator difference between platforms. Let me know if you would prefer a patch like that over the patch clj-1076-fix-tests-on-windows-patch-v2.txt

Comment by Alex Miller [ 02/Sep/13 10:44 PM ]

Marking triaged.

Comment by Alex Miller [ 13/Sep/13 8:50 AM ]

Andy, what if we changed the current patch to replace "\r\n" -> "\n" instead of "\r" -> ""? That way we wouldn't be accidentally removing \r inside lines (which admittedly would be very unusual). I'd be good to go with that change (unless there is a good reason not to do that).

Comment by Andy Fingerhut [ 13/Sep/13 9:16 AM ]

clj-1076-v3.txt is like the other patches, except it uses split-lines on the two multi-line strings to be compared in the test, and compares the result sequences of strings. split-lines splits strings on the regex #"\r?\n", so only where there are newline characters, and optionally a carriage return preceding the newline.

Generated at Sat Oct 21 16:25:42 CDT 2017 using JIRA 4.4#649-r158309.