test.check

the clojure-test namespace has a global atom

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:

Description

There's a global atom in the clojure-test namespace, for tracking something or other across calls to the report functions I think.

This might be problematic for an implementation of parallel tests. I'm not sure, I think it would depend on the details of that implementation.

Activity

Hide
Nicolás Berger added a comment -

The atom is there as part of the trial-report-periodic implementation, it's not used for anything else. I think we could reimplement trial-report-function under the new reporter-fn mechanism, with a function that encloses the atom, thus avoiding having a global atom.

Show
Nicolás Berger added a comment - The atom is there as part of the trial-report-periodic implementation, it's not used for anything else. I think we could reimplement trial-report-function under the new reporter-fn mechanism, with a function that encloses the atom, thus avoiding having a global atom.
Hide
Unnikrishnan Geethagovindan added a comment -

The atom being set when `:being-test-var` is called and used in the `trial-report-periodic` ? Wouldn't the atom have to be global for it to work ?

This might be a silly question (considering I'm fairly new to clojure), How can this be implemented without the atom being global ? If someone can point me in the right direction, I can try to fix this.

Show
Unnikrishnan Geethagovindan added a comment - The atom being set when `:being-test-var` is called and used in the `trial-report-periodic` ? Wouldn't the atom have to be global for it to work ? This might be a silly question (considering I'm fairly new to clojure), How can this be implemented without the atom being global ? If someone can point me in the right direction, I can try to fix this.
Hide
Nicolás Berger added a comment -

The atom being set when `:being-test-var` is called and used in the `trial-report-periodic` ? Wouldn't the atom have to be global for it to work ?

You are totally right: to reset the atom in :begin-test-var the atom needs to be global. But if we find how to reset the atom in a way that it doesn't need to be global, we could make it non-global . I'm thinking of something like:

(defn make-periodic-reporter-fn
  ([] (make-periodic-reporter-fn *trial-report-period*))
  ([report-period] (make-periodic-reporter-fn report-period default-reporter-fn))
  ([report-period reporter]
   (let [last-trial-report (atom 0)]
     (fn [m]
       (case (:type m)
         :started
         (reset! last-trial-report (get-current-time-millis))

         :trial
         (let [t (get-current-time-millis)]
           (when (> (- t report-period) @last-trial-report)
             (with-test-out*
               (fn []
                 (println "Passing trial"
                          (-> m ::trial first) "/" (-> m ::trial second)
                          "for" (get-property-name m))))
             (reset! last-trial-report t)))
         ;; else
         (reporter m))))))

We would also need to add the following at the beginning of c.t.c/quick-check:

(reporter-fn {:type :started
              :property property
              :num-tests num-tests})

This would allow us to use it as (defspec some-spec {:reporter-fn (make-periodic-reporter-fn)} some-prop)

To make it usable via the *report-trials* mechanism (so it is backwards-compatible), we could do something like:

(def trial-report-periodic
  ;; I'm using (constantly nil) as the underlying reporter-fn because we only need to support the :type ::trial
  ;; message. The other messages are already handled by default-reporter-fn, which is going to call us on :type ::trial
  (make-periodic-reporter-fn *trial-report-period* (constantly nil)))
Show
Nicolás Berger added a comment -
The atom being set when `:being-test-var` is called and used in the `trial-report-periodic` ? Wouldn't the atom have to be global for it to work ?
You are totally right: to reset the atom in :begin-test-var the atom needs to be global. But if we find how to reset the atom in a way that it doesn't need to be global, we could make it non-global . I'm thinking of something like:
(defn make-periodic-reporter-fn
  ([] (make-periodic-reporter-fn *trial-report-period*))
  ([report-period] (make-periodic-reporter-fn report-period default-reporter-fn))
  ([report-period reporter]
   (let [last-trial-report (atom 0)]
     (fn [m]
       (case (:type m)
         :started
         (reset! last-trial-report (get-current-time-millis))

         :trial
         (let [t (get-current-time-millis)]
           (when (> (- t report-period) @last-trial-report)
             (with-test-out*
               (fn []
                 (println "Passing trial"
                          (-> m ::trial first) "/" (-> m ::trial second)
                          "for" (get-property-name m))))
             (reset! last-trial-report t)))
         ;; else
         (reporter m))))))
We would also need to add the following at the beginning of c.t.c/quick-check:
(reporter-fn {:type :started
              :property property
              :num-tests num-tests})
This would allow us to use it as (defspec some-spec {:reporter-fn (make-periodic-reporter-fn)} some-prop) To make it usable via the *report-trials* mechanism (so it is backwards-compatible), we could do something like:
(def trial-report-periodic
  ;; I'm using (constantly nil) as the underlying reporter-fn because we only need to support the :type ::trial
  ;; message. The other messages are already handled by default-reporter-fn, which is going to call us on :type ::trial
  (make-periodic-reporter-fn *trial-report-period* (constantly nil)))
Hide
Unnikrishnan Geethagovindan added a comment -

Yea, this works, with a minor change in println

(:so-far m) "/" (:num-tests m)

Should I submit a patch with these following changes incorporated along with corresponding change in tests ?

Show
Unnikrishnan Geethagovindan added a comment - Yea, this works, with a minor change in println
(:so-far m) "/" (:num-tests m)
Should I submit a patch with these following changes incorporated along with corresponding change in tests ?
Hide
Nicolás Berger added a comment -

Thanks for the fix in the println . Adding a patch sounds good to me, but of course we'll have to hear what @gfredericks thinks about it.

Show
Nicolás Berger added a comment - Thanks for the fix in the println . Adding a patch sounds good to me, but of course we'll have to hear what @gfredericks thinks about it.
Hide
Unnikrishnan Geethagovindan added a comment -

Just realised that we will need to add :type ::started to the default-reporter-fn so that it is backward-compatible !

Show
Unnikrishnan Geethagovindan added a comment - Just realised that we will need to add :type ::started to the default-reporter-fn so that it is backward-compatible !
Hide
Unnikrishnan Geethagovindan added a comment -

Attaching patch

Show
Unnikrishnan Geethagovindan added a comment - Attaching patch
Hide
Gary Fredericks added a comment -

I apologize for the extreme delay in following up on this.

Reviewing the patch has made me come to realize that I don't think this problem is solvable.

What I was hoping was that there would be a way to at least have a new atom for each run of quick-check, so that, e.g., running tests in different threads wouldn't cause weird results. The patch does make the atom not globally visible/editable, but (if I'm reading it correctly) there's still just one atom.

And if we're going to maintain the API documented in the *report-trials* docstring, I don't think there's any way around this situation.

So it will probably have to stay as-is until maybe there's a clojure-test-2 namespace some day.

I apologize for not being more detailed in the description, or at least following the conversation in real time to prevent unnecessary work. I definitely appreciate the contribution.

Show
Gary Fredericks added a comment - I apologize for the extreme delay in following up on this. Reviewing the patch has made me come to realize that I don't think this problem is solvable. What I was hoping was that there would be a way to at least have a new atom for each run of quick-check, so that, e.g., running tests in different threads wouldn't cause weird results. The patch does make the atom not globally visible/editable, but (if I'm reading it correctly) there's still just one atom. And if we're going to maintain the API documented in the *report-trials* docstring, I don't think there's any way around this situation. So it will probably have to stay as-is until maybe there's a clojure-test-2 namespace some day. I apologize for not being more detailed in the description, or at least following the conversation in real time to prevent unnecessary work. I definitely appreciate the contribution.

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: