cemerick / clojurescript.test

A maximal port of `clojure.test` to ClojureScript. DEPRECATED
165 stars 32 forks source link

Getting ":optimizations :none" to work #75

Closed mike-thompson-day8 closed 9 years ago

mike-thompson-day8 commented 10 years ago

The project Readme says:

clojurescript.test supports all of Google Closure's compilation modes, including :advanced

But that's untrue. This framework does not handle :optimizations :none. I tried. It failed. I researched. I figured out it was impossible.

But if I retreat to, say, :optimizations :whiteapce the compile time delays get in the way of a good fast iterative workflow. I needed the near-instant compile time speed of :optimizations :none !!

In the end, I worked out a method, which I documented here:
https://github.com/mike-thompson-day8/cljsbuild-none-test-seed

Feel free to use the material in there as you see fit. For example, the repo contains a specific phantomjs runner for this :none setting which you may find easy to incorporate. And the test.html will be of interest too, I believe.

I didn't want to produce a patch because I didn't know what (if anything) you'd want to use and how.

mike-thompson-day8 commented 10 years ago

This issue is considered a show stopper for wider adoption: https://github.com/swannodette/mies-node-template/pull/1#issuecomment-53449126

cemerick commented 10 years ago

If you can produce a patch that makes clojurescript.test work with :none, I'm interested. Doing so sanely will require factoring out the dependency-loading code into something that can be used from the multiple runners.

To be clear, I'm happy to take the patch; it's just not something I can spend time on. Thanks for prototyping to give everyone hope. ;-)

mike-thompson-day8 commented 10 years ago

I don't see how I the dependency-loading code can be factored out. We've done two runners (one for phantomjs and one for nodejs) and they have to jump through quite different hoops. Very little in common to be factored out.

Here is what I propose: in addition to the existing :runner and :node-runner options, I would propose adding :runner-none and :node-runner-none (note that -none has simply been appended to the names of the existing runners to handle the :optimisations :none case).

Would that be acceptable?

Also, I've done a test.html version of the runner which allows for debugging of unit tests in the browser. I'd like to incorporate that. It is less clear to me where that should go. Any thoughts?

cemerick commented 10 years ago

Unless there's a really good reason, I don't want to grow the set of runner scripts; that's more user-facing complexity and just adds to the maintenance burden of keeping the scripts up to date and in sync w.r.t. bug fixes, etc.

Browser targeting is a whole 'nuther can of worms. There's already a couple of issues (see #21 and #48), though no one has dug into it too deeply. Maybe open a new isue describing your approach and which use case(s) it addresses?

mike-thompson-day8 commented 10 years ago

I understand your objective, I really do. It would be good, from a user perspective, to reduce the number of runners, and to get the selection more automatic

BUT, by the same logic, there shouldn't currently be different runners for nodejs and phantomjs either.

But there are. And that's because in these different contexts, there's quite a different set of actions required. Not a lot can be factored out. And the script itself would have to detect the context in which it is running. That leads to a complicated script, edge cases, etc!!

And here's the thing: it is pretty much the same story between :optimizations :none and the rest of the settings :simple whitepace etc. In fact, I would argue that it is worse.

First, there's a quite different set of actions required for :none. (which is why it hasn't been solved previously as far as i know).

Second, how could the runner script tell if the cljsbuild config has :optimisations :none or another setting? It would have to read the project.clj file? Or perhaps it should look for certain patterns on the inside of the file given via :output-to? All sounds pretty brittle.

Even the commandline for the scripts will be different, much less the contents. In the :none case you need to know the :output-dir and ::output-to settings (because you will be incrementally loading js files from the inside of that output-to directory).

Compared to all that potential mess, I thought the "different runners" solution looked pretty easy and elegant. Especially because I could have that patch done by the end of today.

No matter what happens (I'm beginning to think this isn't going to happen), I think its important that the README is changed to say that this library absolutely can't currently be used with :optimisations :none. As noted in my initial ticket text (up the top), the current docs are misleading and that leads to people wasting time. I know I wasted time.

mike-thompson-day8 commented 10 years ago

Regarding your question as to "what use case" my browser-based solution addresses, I'd simply point to this issue's heading.

It does the same as the phantomjs "runner" except it runs in the browser. No more or less. You see the same output in the browser as you would if you'd run in phantomjs (displayed in HTML, rather than the commandline). But, because it is in browser, you can debug more easily.

Being able to use :optizations :none and have it work in the browser is the win.

martinklepsch commented 9 years ago

Reading CLJS-851 I'm under the impression that this could be fixed by the solution proposed in the ticket?

EDIT The basic proposition is to compile a single JS file that loads all the different files that need to be loaded when using :optimisations :none.

mike-thompson-day8 commented 9 years ago

@martinklepsch unfortunately CLJS-851 won't help. When unit-testing, you have a directory full of test cljs files, and each is a standalone island WRT to its siblings. There's no root to the namespace tree.

Without a root to the tree, there's no one goog.require that pulls everything in. So getting a better way of nominating a :main doesn't help.

https://github.com/mike-thompson-day8/cljsbuild-none-test-seed/blob/master/test.html#L69-L79

martinklepsch commented 9 years ago

@mike-thompson-day8 Ah — I didn't fully understand the problem then.

An option would be to have a separate test-main kind of namespace that requires all tests and potentially the non-testing main namespace. Would it work if you'd pass test-main as :main then?

I'm not sure if that should be the solution though...

EDIT: this obviously wouldn't solve the problem you described in the linked comment:

    // And the list of namespaces changes over time as we add and remove
    // unitest cljs files.  Uggh. Disaster.
mike-thompson-day8 commented 9 years ago

I've added a comment to CLJS-851

cemerick commented 9 years ago

@mike-thompson-day8 If you're still up for it, I'll take a PR for the -none proliferation of test runner scripts. The "ick" factor remains for me, but I'll get over it. :-)

mike-thompson-day8 commented 9 years ago

@cemerick I'm about to disappear on a holiday for 3 weeks. When I get back, it will take me a while to get some spare time. But happy to pull something together when I get through all that. Just wanted to let you know timings.

cemerick commented 9 years ago

No worries, I appreciate the efforts, whenever they come.

moea commented 9 years ago

I have a runner I'm using with none, if anyone's still interested in having this be supported.

martinklepsch commented 9 years ago

@moea definitely share it! :)

moea commented 9 years ago

@martinklepsch https://gist.github.com/moea/ed4d4f428a4cb9905c24

Leaving it as a gist, for now - there's some stuff I'd add in before submitting a PR, to make it behave more like the regular runner (support for addition of arbitrary code/shims, more error checking - I wanted it to be as brief as possible, and don't need those features).

nodejs runner-none.js cljsbuild-output-dir cljsbuild-output-file

bensu commented 9 years ago

:optimizations :none works now in doo

cemerick commented 9 years ago

Sorry for the troubles, but I've deprecated this project. Please see the notice at the top of the repo's README. This is a good thing, fundamentally. :-)