-
Notifications
You must be signed in to change notification settings - Fork 115
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
Issues 296 and 300 - Add a macro for common tests between Clojure and ClojureScript #301
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
(ns clara.tools.testing-utils | ||
"Internal utilities for testing clara-rules and derivative projects. These should be considered experimental | ||
right now from the perspective of consumers of clara-rules, although it is possible that this namespace | ||
will be made part of the public API once its functionality has proven robust and reliable. The focus, however, | ||
is functionality needed to test the rules engine itself." | ||
(:require [clara.macros :as m] | ||
[clara.rules.dsl :as dsl] | ||
[clara.rules.compiler :as com])) | ||
|
||
(defmacro def-rules-test | ||
"This macro allows creation of rules, queries, and sessions from arbitrary combinations of rules | ||
and queries in a setup map without the necessity of creating a namespace or defining a session | ||
using defsession in both Clojure and ClojureScript. The first argument is the name of the test, | ||
and the second argument is a map with entries :rules, :queries, and :sessions. For example usage see | ||
clara.test-testing-utils. Note that sessions currently can only contain rules and queries defined | ||
in the setup map; supporting other rule sources such as namespaces and defrule/defquery may be possible | ||
in the future. | ||
|
||
Namespaces consuming this macro are expected to require clara.rules and either clojure.test or cljs.test. | ||
Unfortunately, at this time we can't add inline requires for these namespace with the macroexpanded code in | ||
ClojureScript; see https://anmonteiro.com/2016/10/clojurescript-require-outside-ns/ for some discussion on the | ||
subject. However, the test namespaces consuming this will in all likelihood have these dependencies anyway | ||
so this probably isn't a significant shortcoming of this macro." | ||
[name params & forms] | ||
(let [sym->rule (->> params | ||
:rules | ||
(partition 2) | ||
(into {} | ||
(map (fn [[rule-name [lhs rhs props]]] | ||
[rule-name (dsl/parse-rule* lhs rhs props {})])))) | ||
|
||
sym->query (->> params | ||
:queries | ||
(partition 2) | ||
(into {} | ||
(map (fn [[query-name [params lhs]]] | ||
[query-name (dsl/parse-query* params lhs {})])))) | ||
|
||
production-syms->productions (fn [p-syms] | ||
(map (fn [s] | ||
(or (get sym->rule s) | ||
(get sym->query s))) | ||
p-syms)) | ||
|
||
session-syms->session-forms (->> params | ||
:sessions | ||
(partition 3) | ||
(into [] | ||
(comp (map (fn [[session-name production-syms session-opts]] | ||
[session-name (production-syms->productions production-syms) session-opts])) | ||
(map (fn [[session-name productions session-opts]] | ||
[session-name (if (com/compiling-cljs?) | ||
(m/productions->session-assembly-form (map eval productions) session-opts) | ||
`(com/mk-session ~(into [(vec productions)] | ||
cat | ||
session-opts)))])) | ||
cat))) | ||
|
||
test-form `(~(if (com/compiling-cljs?) | ||
'cljs.test/deftest | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is sort of a macro-anti-pattern to return code that has implicit dependencies on namespaces being pre-required by the caller. If this macro was used by a caller that did not have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I discussed this some in the docstring here. I agree that it is undesirable; unfortunately, my understanding is that ClojureScript doesn't have good support for macros causing additional namespaces to be required since all requires have to be at the beginning of the file. I'd be happy to find out I'm wrong about that or that there is a way around this problem though, and I'm certainly not as well-versed in ClojureScript's compilation model as in Clojure's. Practically speaking, for this particular use-case it probably isn't a big deal since I'd expect the namespaces consuming this to have those requires anyway and this is currently targeted toward consumption within Clara, not as an external API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, wow, you explained this already thoroughly in the doc string. I somehow overlooked it. It'd be pretty tricky to pull of the require on the cljs side I'd think so seems fine for now. |
||
'clojure.test/deftest) | ||
~name | ||
(let [~@session-syms->session-forms | ||
~@(sequence cat sym->query)] | ||
~@forms))] | ||
test-form)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
#?(:clj | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is somewhat non-idiomatic to use a read conditional at this scope. It is more common to see them used just in the places where the forms differ. Here there is duplication and even a chance the It's not a huge issue though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, doing it all in one body with reader conditionals is also an option. In this case I just thought that so much differed for such a small ns header that it would be more readable to just have entirely separate forms. Looking back at it I'm thinking perhaps make the namespace name common and just have the rest be separate. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having everything other than the name be separate is complicated by having multiple top-level forms beneath the declaration for clj and cljs. I think I'll just keep this as is; by my count only the clara.rules require is identical between clj and cljs so splicing in lots of reader conditions would hurt readability more than it would help it in this instance IMO. |
||
(ns clara.test-testing-utils | ||
(:require [clara.tools.testing-utils :refer [def-rules-test]] | ||
[clara.rules :as r] | ||
|
||
[clara.rules.testfacts :refer [->Temperature ->Cold]] | ||
[clojure.test :refer [is deftest run-tests] :as t]) | ||
(:import [clara.rules.testfacts | ||
Temperature | ||
Cold])) | ||
|
||
:cljs | ||
(ns clara.test-testing-utils | ||
(:require [clara.rules :as r] | ||
[clara.rules.testfacts :refer [->Temperature Temperature | ||
->Cold Cold]] | ||
[cljs.test :as t]) | ||
(:require-macros [clara.tools.testing-utils :refer [def-rules-test]] | ||
[cljs.test :refer (is deftest run-tests)]))) | ||
|
||
(def test-ran-atom (atom false)) | ||
|
||
;; This test fixture validates that def-rules-test actually executed the test bodies it | ||
;; is provided. If the test bodies were not executed test-ran-atom would have a value of false | ||
;; after test execution. | ||
(t/use-fixtures :once (fn [t] | ||
(reset! test-ran-atom false) | ||
(t) | ||
(is (true? @test-ran-atom)))) | ||
|
||
(def-rules-test basic-tests | ||
{:rules [rule1 [[[?t <- Temperature (< temperature 0)]] | ||
(r/insert! (->Cold (:temperature ?t)))]] | ||
|
||
:queries [query1 [[] | ||
[[Cold (= ?t temperature)]]]] | ||
|
||
:sessions [session1 [rule1 query1] {} | ||
session2 [rule1 query1] {:fact-type-fn (fn [fact] :bogus)}]} | ||
|
||
(reset! test-ran-atom true) | ||
(is (= [{:?t -50}] | ||
(-> session1 | ||
(r/insert (->Temperature -50 "MCI")) | ||
r/fire-rules | ||
(r/query query1)))) | ||
|
||
;; Since we validate later (outside the scope of this test) that the state | ||
;; change occurred put it in the middle so that it would fail if we took either | ||
;; the first or last test form, rather than all test forms. | ||
(reset! test-ran-atom true) | ||
|
||
(is (empty? (-> session2 | ||
(r/insert (->Temperature -50 "MCI")) | ||
r/fire-rules | ||
(r/query query1))))) |
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.
It does seem odd to me that the clj impl would embed a class object in the data structure at this point. A fully-qualified symbol seems more appropriate and a "data representation". The fact that clj lets you embed class objects in
eval
code etc just seems to be non-documented behaviors.This is a broader topic though, so not really suitable to be fixed in this issue.
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 it is a bit odd but probably outside the scope of #296 or #300
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, we can drop this in another change. I originally had wanted to validate the presence of a type early in the process to fail fast, but it doesn't make sense to include in our structure.