Clojure

pprint tests fail on Windows, expecting \n

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Completed
  • Affects Version/s: Release 1.5
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
  • Environment:
    Windows 7
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

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

  1. clj-1076-fix-tests-on-windows-patch-v1.txt
    29/Sep/12 2:27 PM
    3 kB
    Andy Fingerhut
  2. clj-1076-fix-tests-on-windows-patch-v2.txt
    02/Mar/13 2:50 PM
    2 kB
    Andy Fingerhut
  3. clj-1076-v3.txt
    13/Sep/13 9:16 AM
    2 kB
    Andy Fingerhut
  4. pprint_test_failures_01b4cb7156.txt
    26/Sep/12 2:34 AM
    25 kB
    Ivan Kozik

Activity

Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Alex Miller added a comment -

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).

Show
Alex Miller added a comment - 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).
Hide
Alex Miller added a comment -

Marking triaged.

Show
Alex Miller added a comment - Marking triaged.
Hide
Andy Fingerhut added a comment -

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

Show
Andy Fingerhut added a comment - 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
Hide
Alex Miller added a comment -

Is this still happening on Windows?

Show
Alex Miller added a comment - Is this still happening on Windows?
Hide
Andy Fingerhut added a comment -

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

Show
Andy Fingerhut added a comment - Presumptuously changing Approval from Incomplete back to None, since there is a new patch attached that should address the reason it was marked Incomplete.
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Stuart Halloway added a comment -

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.

Show
Stuart Halloway added a comment - 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.
Hide
Mike Anderson added a comment -

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....

Show
Mike Anderson added a comment - 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....
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Mike Anderson added a comment -

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

Show
Mike Anderson added a comment - 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
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Ivan Kozik added a comment - - edited

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">
<classpath>
+ <pathelement path="${java.class.path}"/>
<pathelement path="${maven.test.classpath}"/>

Show
Ivan Kozik added a comment - - edited 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"> <classpath> + <pathelement path="${java.class.path}"/> <pathelement path="${maven.test.classpath}"/>
Hide
Andy Fingerhut added a comment -

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?

Show
Andy Fingerhut added a comment - 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?

People

Vote (1)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: