search

weavejester / ragtime

Database-independent migration library
Eclipse Public License 1.0
610 stars 85 forks source link

Incorporate fix for issue 103. Fix tests to have platform independent… #121

Closed jconti closed 5 years ago

jconti commented 6 years ago

Please verify this patch on the Unix or MacOS box you are using. I fixed and ran the test on Windows 10. If there is any issue I do have access to MacOS and Linux platforms to fix and retest. I would be happy to do that.

weavejester commented 6 years ago

Why are the format changes necessary?

jconti commented 6 years ago

The strings in the tests contained line endings. Those are new lines on most operating systems, but on windows are carriage-returns followed by new lines.

The format syntax “%n” expands to whatever the platform uses for a line end. It seemed like the smallest change that would make the tests cross-platform.

This choice won’t work on Clojurescript without adding a dependency that implements (format) so let me know if that is an issue (node.js in the future maybe?)

weavejester commented 6 years ago

My understanding is that Java knows enough about the platform it's running on to translate \n to \r\n without additional code. Can you check and remove the formats if this is the case?

jconti commented 6 years ago

There are other ways to remove the formats. I chose to put them in because the tests fail on Windows without them, and I thought they were the least invasive. I think your understanding is generally correct about Java, as Windows was always a first-class citizen for the implementation team.

The nuance here is that Java has features for platform independent line ends, and features to support software that needs to interact with the platform and be aware of platform-specific line endings. This patch applies to code using platform-specific as well as platform-independent JVM line ending features, as follows:

  1. First of all the actual bug was caused by using platform-specific line ending features on URI's which are normalized and platform-independent (jdbc.clj:97). The regular expression that uses File/separator is trying to use the platform specific file separator character on URIs which only use '/' by definition (no matter what the platform). So the patch removes those platform-specific characters from the pattern for basename.

  2. With the above change made, the convention is now established that basename should only operate on URI's like the ones returned from load-resources. However, load-directory does not return URIs but rather filenames. So now the basename fn is broken for load-directory on Windows. But we can keep the changes so far and stay platform independent by making sure that load-directory returns URIs just like load-resources does. And that is the change in jdbc.clj:143.

  3. Now the platform-specific static fields from java.io.File are no longer needed. The code in jdbc.clj does not interact with the JVM in any platform specific way now.

So that is the good news, I think the code is improved and basename has a consistent input type. It may be that this relationship should be more concrete in the code by indicating a URI is expected. That could be as simple as naming the parameter uri, or as complicated as making the inputs ^java.net.URI. I do not have an opinion on that.

  1. However tests fail on Windows. The root cause is that some tests used with-out-str to capture string output from the reporter. This seems reasonable, however the strings returned from with-out-str have platform-specific line endings in them. with-out-str is capturing strings as they are produced through a writer interface that is taking '\n' and mapping that to the writer's newline method. This allows strings that use '\n' to automatically produce appropriate line endings for the platform they are running on. It is a useful feature that allows JVM programmers to usually produce the right (platform-specific) line ending without even thinking about it.

  2. But the dark side of this magic interpolation of '\n' by the Windows writer to '\r\n' is that the string "Applying foo\n" will match the output in `print-reporter-test on MacOs and (Li)|Unix but not on Windows. On Windows the same string will be "Applying foo\r\n". This is the point that confuses the heck out of people often. It causes the test failures because (not= "Applying foo\n" "Applying foo\r\n").

  3. Many schemes are used to handle this. Single line output is often just run through something like clojure.string/trimr or clojure.string/trim-newline. This will not work for multi-line strings. Lots of solutions can fix that, normalizing by replacing '\r' with nothing unconditionally before compare, etc. I chose format because it explicitly has syntax for generating platform specific line endings like the writer interface does. I thought using format was the smallest, simplest, most explicit about its intent change that still completely matched the test output and so changed no meaning of the tests at all.

  4. However, I freely admit

    • The change is not very pretty (but it is only in the tests)
    • %n is not very readable (but it is explicit in its intent, not magical like \n inconsistently is)
    • Is not portable to javascript vms (but uses only core functions).
    • Changes literal strings into function calls on strings (but does not make any test "helper" functions that have to be used everywhere in the tests to make them work. Those often get forgotten, and then Windows tests will randomly break in the future as people forget).

Hope this explanation helps more than it hurts 8^).

jconti commented 6 years ago

I just thought of something that might be clear and simple, with no format. Let me know what you think:

Instead of:

(is (= (with-out-str
               (migrate-all database {} migrations
                            {:strategy strategy/rebase, :reporter reporter/print}))
         (format "Applying assoc-x%nApplying assoc-y%n")))

Maybe this?

(is (= (with-out-str
               (migrate-all database {} migrations
                            {:strategy strategy/rebase, :reporter reporter/print}))
         (with-out-str 
               (print "Applying assoc-x\nApplying assoc-y\n")))

Or?

(is (= (with-out-str
               (migrate-all database {} migrations
                            {:strategy strategy/rebase, :reporter reporter/print}))
         (with-out-str 
               (println "Applying assoc-x")
               (println "Applying assoc-y")))
IamDrowsy commented 6 years ago

Would it be possible to apply the fix for #103 and if it's controversy without the fixes for the tests (and deal with them in a separate issue)? It's not like the patch itself breaks the test, they are already not working on windows. And while having broken tests on a specific platform is not nice, at least ragtime would be working on windows without any runtime hacks.

weavejester commented 6 years ago

Apologies; this fell through my inbox "todo" list.

@jconti the with-out-str is a nice and elegant solution to the problem. If you implement that, I'd be happy to merge.

jconti commented 6 years ago

Will do.

On Oct 20, 2018, at 7:56 AM, James Reeves notifications@github.com wrote:

Apologies; this fell through my inbox "todo" list.

@jconti the with-out-str is a nice and elegant solution to the problem. If you implement that, I'd be happy to merge.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jconti commented 6 years ago

In reply to https://github.com/weavejester/ragtime/pull/121#issuecomment-431584114:

The code uses with-out-str. It replaces \n with %n in calls to format (which I agree is ugly). In reviewing this objection, I do not know a better way to do that. To summarize my wall of text (apologies):

Unix

(map char-escape-string (System/lineSeparator)) => ("\\n")

Windows

(map char-escape-string (System/lineSeparator)) => ("\\r" "\\n")

weavejester commented 6 years ago

I'm not sure what you're saying in your previous comment. What wall of text are you summarizing?

Now that I'm caught up on the necessity of format, I'll accept the PR as-is or with with-out-str. Either will do. Just make sure the commit message is changed to follow the seven rules and I think we're fine to merge.

jconti commented 5 years ago

Apologies it took me so long just to edit a commit message! :-)

weavejester commented 5 years ago

No problem. Can you squash your commits? You seem to have added a new commit, then added a merge commit, instead of amending the commit message of your original commit.

jconti commented 5 years ago

You bet. I am a ham-fisted git user ;-)

On Nov 16, 2018, at 6:06 PM, James Reeves notifications@github.com wrote:

No problem. Can you squash your commits? You seem to have added a new commit, then added a merge commit, instead of amending the commit message of your original commit.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/weavejester/ragtime/pull/121#issuecomment-439573525, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJ0sFRktupv2EtigFol-HQFb6Su_Enlks5uv2EUgaJpZM4RHqio.

jconti commented 5 years ago

I think that should do it, however, don't hesitate to point out anything requiring additional work.