-
-
Notifications
You must be signed in to change notification settings - Fork 181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make pod tests pass, and run them from make
#567
Conversation
Hrm, I take it Appveyor runs on Windows... |
Great thanks! |
alright, my AppVeyor fix worked :) |
@@ -21,7 +21,7 @@ | |||
(def ^:dynamic *sync-delete* true) | |||
(def ^:dynamic *hard-link* true) | |||
|
|||
(def windows? (boot.App/isWindows)) | |||
(def ^:dynamic windows? (boot.App/isWindows)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is dynamic
we might as well rename it to *windows?*
maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the change I was most unsure about. The only reason for windows?
to be dynamic is for testing purposes, and given that's the case, I didn't think that advertising its dynamic-ness with asterisks was a great idea.
At the time, I didn't know that CI ran on Windows as well, and that fact makes me think there's a better way to do this test? https://github.com/boot-clj/boot/pull/567/files#diff-b6e908691c9bd8d211924e12cfbbcd61R46
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean it is just a minor thing anyways, given that we don't know all the implications here we might need to wait better opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree. I'm working on a better opinion myself :)
@@ -22,7 +22,7 @@ | |||
|
|||
(deftest relative-to-test | |||
(testing "Nil base" | |||
(is (= "out/goog/base.js" (str (relative-to nil (io/file "out/goog/base.js")))))) | |||
(is (thrown? AssertionError (str (relative-to nil (io/file "out/goog/base.js")))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhm, this one is odd to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... nil
is covered by a :pre
condition in relative-to
. I thought about removing the test, but wasn't sure if there was a good reason for it to be there. I think it's the only other option though, unless the :pre
shouldn't be there.
Looks very good to me :) |
Relying on `clojure.java.io/file`'s ability to construct paths with platform-specific separators is better than making `boot.file/windows?` dynamic. Also, there's no need to test for nil when there's a precondition guarding against it.
Okay @arichiardi, I hope you'll like this. It occurred to me that I also removed the test for AssertionError, since the |
Perfect! I think now it looks way way simpler. Maybe the original author of this ns can also chime in. In any case, good job! |
@arichiardi I just realized github is still saying that you're requesting changes. Is that still true? If so, I'm not sure what they are. |
Uhm no, i am good, let me see if I can cancel the review. |
Fantastic, thanks :) |
🤘 🎸 🤘 |
Problems with these tests were discovered while working on #566, so I figured I'd take a shot at fixing them. I elected to change the code as little as possible, and change the tests instead, but I'm not super confident I made the right decision in every case. Criticism welcome.