Add meta checks #59

jackfirth · 2017-07-06T08:35:22Z

Closes #47

This PR adds the following checks:

(check-fail <pred-or-exn> <thunk>) - asserts that <thunk> evaluates a failing check and that the check failure exception satisfies <pred-or-exn> in the same way as check-exn
(check-fail* <thunk>) - like check-fail, but without caring about the specifics of the thrown failure
(check-fail/info <check-info> <thunk>) - asserts that <thunk> evaluates a failing check and that the check failure exception contains an info that is equal? to <check-info>
(check-error <pred-or-exn> <thunk>) - asserts that <thunk> evaluates a check that raises an error instead of failing and that the raised value satisfies <pred-or-exn>

I went with check-fail/info instead of adding an info case to check-fail to avoid confusion over whether check-error has an info-accepting variant. The existence of check-fail/info and nonexistence of check-error/info is a clear signal in documentation search results and playing at the REPL that wouldn't be present if check-fail/info was merged with check-fail.

The only addition from the proposed API is check-error, which I realized I'd need to test to the contract failure cases of these checks.

AlexKnauth · 2017-07-06T17:20:27Z

Why is check-error called check-error?

jackfirth · 2017-07-06T17:44:29Z

Symmetry with the output of checks, since check failures print "FAILURE" while check errors print "ERROR". If you have a suggestion for a less ambiguous name, I'm all ears.

bennn · 2017-07-06T18:11:11Z

How does check-error differ from check-exn?

I tried a quick example and check-exn works how I'd expect:

#lang racket
(module+ test
  (require rackunit)
  (check-exn exn:fail:contract?
    (lambda () (check-true (+ "asdf" 2)))))

$ raco test test.rkt
raco test: (submod "test.rkt" test)
1 test passed

Reminds me though, it would be good to have a check-exn helper to test "throws a contract error that blames a certain party"

jackfirth · 2017-07-06T18:27:07Z

That example works because (+ "asdf" 2") is evaluated before being passed as an argument to check-true, so the error isn't thrown from within the body of check-true. Try this example:

(check-exn (lambda (v) (equal? v 'oops))
  (lambda () (check-pred (lambda (v) (raise 'oops)) 'foo)))

bennn · 2017-07-07T03:31:43Z

rackunit-lib/rackunit/private/meta.rkt

+  (with-expected pred-or-msg
+    (assert-failure failure)
+    (assert-check-failure failure)
+    (if (procedure? pred-or-msg)


I think (predicate/c pred-or-msg) will do the right thing (and also check (procedure-arity-includes? pred-or-msg 1))

(or use contract-out)

~~can you add a procedure-arity-includes? check here?~~

bennn · 2017-07-07T03:32:07Z

rackunit-lib/rackunit/private/meta.rkt

+        (unless (pred-or-msg failure)
+          (fail-check "Check failure didn't pass predicate"))
+        (unless (regexp-match? pred-or-msg (exn-message failure))
+          (fail-check "Check failure message didn't match regexp")))))


These fail-check should print failure somehow. (Do they?)

I'd prefer they mirror check-exn, and also say "Wrong failure raised" in both cases

Done. They now print the message and the failure value itself, just like check-exn.

bennn · 2017-07-07T03:32:18Z

rackunit-lib/rackunit/private/meta.rkt

+        (unless (equal? info expected-info)
+          (with-actual info
+            (fail-check
+             "Check failure contained an unexpected info value")))))))


This stops at the first unexpected info, right?

I think it'd be better to print all the unexpecteds.

I hadn't thought about if the info stack contains multiple infos with the expected name but different values. This needs to be reworked to deal with that.

bennn · 2017-07-07T03:32:26Z

rackunit-lib/rackunit/private/meta.rkt

+    (assert-failure failure)
+    (assert-not-check-failure failure)
+    (cond
+      [(procedure? pred-or-msg)


(predicate/c pred-or-msg)

That's a higher order contract, so I don't think it would work here.

same here, add procedure-arity-includes?

bennn · 2017-07-07T03:32:33Z

rackunit-lib/rackunit/private/meta.rkt

+(define-simple-macro (with-expected exp:expr body:expr ...)
+  (with-check-info* (list (make-check-expected exp)) (λ () body ...)))
+
+;; Pseudo-contract helpers, to be replaced with real check contracts eventually


eventually = this pull request?

By "real check contracts", I mean #55. I don't plan on implementing those in this PR.

bennn · 2017-07-07T03:32:36Z

rackunit-lib/rackunit/private/meta.rkt

+;; Pseudo-contract helpers, to be replaced with real check contracts eventually
+
+(define (contract-pred-or-msg! name pred-or-msg)
+  (unless (or (procedure? pred-or-msg) (regexp? pred-or-msg))


predicate/c
(next line too)

I'd rather not add the complexity of real higher order contracts here. Additionally, the function used as a predicate can return non-boolean values which predicate/c doesn't allow.

ok. then (and (procedure? pred-or-msg) (procedure-arity-includes? pred-or-msg 1))
?

bennn · 2017-07-07T03:33:12Z

rackunit-lib/rackunit/private/meta.rkt

+  (parameterize ([current-check-handler raise]
+                 [test-log-enabled? #f]
+                 [current-check-info (list)])
+    (with-handlers ([(λ (e) (not (exn:break? e))) values]) (chk-thnk) #f)))


use negate ?

I don't really care but racket/function's already imported

jackfirth · 2017-07-13T05:46:43Z

@bennn Added docs and addressed all your comments (I think), this PR is ready for a final review.

bennn

good timing :)

bennn · 2017-07-13T05:58:39Z

rackunit-doc/rackunit/scribblings/check.scrbl

+                     [message string? ""])
+         void?]{
+ Checks that @racket[thunk] evaluates a failing check and that
+ @racket[fail-exn-predicate], it it's a function, returns a true value when


it it's => if it's

bennn · 2017-07-13T06:02:28Z

rackunit-doc/rackunit/scribblings/check.scrbl

+ @racket[fail-check], not the optional @racket[message] argument that all checks
+ accept. See also @racket[check-exn] and @racket[check-error].
+
+ @(interaction


Should get the same output with @examples[#:label #f ....

Done. Didn't add #:label #f because using the label seems appropriate. They are examples, after all.

bennn · 2017-07-13T06:09:39Z

rackunit-doc/rackunit/scribblings/check.scrbl

+                      [message string? ""])
+         void?]{
+ Checks that @racket[thunk] evaluates a check that raises an error value instead
+ of passing or failing, and checks that the raised value, if it it's a function,


if it => if

Also "checks that the raised value" should refer to fail-exn-predicate.

(The sentence is a little long & awkward ... maybe start a new sentence after "passing or failing"? or end it with "checks that the raised value satisfies the predicate. Satisfies means ....")

bennn · 2017-07-13T06:11:58Z

rackunit-lib/rackunit/private/meta.rkt

+  (with-expected pred-or-msg
+    (assert-failure failure)
+    (assert-check-failure failure)
+    (if (procedure? pred-or-msg)


~~can you add a procedure-arity-includes? check here?~~

bennn · 2017-07-13T06:12:10Z

rackunit-lib/rackunit/private/meta.rkt

+    (assert-failure failure)
+    (assert-not-check-failure failure)
+    (cond
+      [(procedure? pred-or-msg)


same here, add procedure-arity-includes?

AlexKnauth · 2017-07-13T06:25:19Z

Would it be better if check-error were renamed to either check-check-error or check-fail-error?

jackfirth · 2017-07-13T06:31:38Z

@AlexKnauth How about check-fail/error? According to scheme pronunciation rules, that's "check fail with error" which seems accurate.

jackfirth · 2017-07-13T06:33:06Z

@bennn For some reason I can't respond directly to your comments about procedure-arity-includes?. I added the arity check in contract-pred-or-msg! so I don't think it needs to be tested in the cond expression.

AlexKnauth · 2017-07-13T06:35:24Z

(Re: check-fail/error) That makes sense to me.

rmculpepper · 2017-07-13T21:48:49Z

I think this should be a new module, maybe rackunit/meta. That would make it clearer that these are not general-purpose checks, but rather specialized for testing other checks.

I'd prefer if check-fail, check-fail*, and check-fail/info were combined into one form. (I disagree with your rationale for keeping them separate.) I propose:

;; check-fail : (Treeof FailurePred) (-> Any) -> Void
;; where FailurePred is one of
;; - (exn:test:check -> Boolean)  -- exn must satisfy predicate
;; - Regexp                       -- exn message must match
;; - Check-Info                    -- exn must contain check-info

More descriptive, less ambiguous names would be check-check-fail and check-check-error... but I could understand not liking them. check-fail/exn is not quite right, since we consider failures distinct from errors. But check-error is bad, because it's too similar to check-exn. (I believe there's a library somewhere that defines check-error as a macro that wraps the expression to check as a thunk.)

jackfirth · 2017-07-15T00:42:38Z

@rmculpepper Thanks for taking a look at this! Here are my thoughts:

A rackunit/meta module is a good idea, but why stop there? Should there be rackunit/base, rackunit/check, rackunit/suite, or other modules for splitting up the contents of rackunit? I'd find it strange to only have rackunit/meta provided from a special module while nothing else in RackUnit's non-UI code is modularized. What about if rackunit provides the meta checks and we open a separate issue for finer-grained modules?

To me, the (Treeof FailurePred) argument API is too complex. It presents multiple ways of doing the same thing, e.g. these are all nearly equivalent:

(test-begin
  (check-fail (list #rx"foo" pred) thnk))

(test-begin
  (check-fail #rx"foo" thnk)
  (check-fail pred thnk))

(test-begin
  (check-fail (list (list #rx"foo") (list (list pred))) thnk))

One of these could be preferred, but will they have subtle differences that a user has to remember? What about the extra location info provided by separate checks? And when using only a single predicate / regex / info, does it needs to be wrapped in a list to be considered a tree? Can you mix predicates / regexes / infos in the same tree? Is the order important? Is the tree traversed depth-first or breadth-first? Why is it a tree and not a list? If multiple parts of the tree fail, which message is shown? These are all questions I'd have to ask myself as a user of this API, and I'd likely forget the answers to some of them frequently and have to consult the docs. Not to mention there's the increased implementation and contract complexity. I don't see the benefits of this approach outweighing the costs.

bennn · 2017-08-01T19:25:39Z

I like @rmculpepper 's suggestions.

:+1: for rackunit/meta, because it distinguishes "RackUnit for the meta-tester" from "RackUnit for the normal user".
- it's fine with me to add rackunit/base etc. as long as (require rackunit) still gives the same imports.
:+1: for having one form
:+1: for accepting a tree. The plot library has a similar (very convenient) API for renderers. I think it should check everything in the tree & report all errors. Order should be unspecified --- if you want to meta-meta-test then only supply non-list arguments

jackfirth · 2017-08-02T23:22:27Z

Re rackunit/meta: I will move these exports to rackunit/meta and not export them from rackunit. This PR will not attempt to add a rackunit/base module or make any changes to the current exports of rackunit.

As for the tree API:

A tree API wouldn't quite be a single form, because it would still need separate forms for normal failures and errors. A third form that that supplied an empty tree by default would also be needed if we wanted to match the simplicity of check-fail*.

A tree of renderers in plot makes sense because there would be a lot of duplication otherwise, and there's no semantic changes that can occur as a result of the order of renderers in the tree. Changing the order of values in the tree supplied to a tree-based meta check form would change which error is shown, and I don't see any circumstance where the message argument to such a check would apply to all leaves of a tree.

A tree API also makes it harder to distinguish between different kinds of failures. Each leaf of the tree would have the same check location information. DrRacket's integration with the location information makes it easy to instantly see exactly which cases failed; I consider it bad practice for a rackunit API to encourage users to subvert that feature. Maybe if we had some way to point to different leaves of the tree in the location message?

bennn · 2017-08-15T16:03:22Z

I still like the tree API

I don't mind losing check-fail*
RackUnit can test everything in the tree (so changing order only changes the orders of errors, not "which errors are shown")
to distinguish kinds of errors, you can split one check-fail* into multiple (check-exn has the same "problem" --- a user can write one big predicate that tests lots of things & is hard to debug)

jackfirth · 2017-08-16T04:31:15Z

I'd like to get this feature in and it seems I'm the only one doubting a tree API, so I shall defer to your judgment. Tree API it is!

With multiple errors, how should the checks report failures? Multiple failures for one check is something I've been working on in racket-expect and it can be complex enough that I'd rather avoid implementing it entirely here.

bennn · 2017-08-18T03:33:54Z

I think just normal messages separated by whitespace would be OK. Example:

--------------------
FAILURE
name:       check-equal?
location:   foo.rkt:4:0
actual:     1
expected:   0

name:       check-equal?
location:   foo.rkt:5:0
actual:     1
expected:   2
--------------------

But since name & location are always the same, maybe each failed check in the tree could register a unique check-info key & all these keys could be printed on failure.

jackfirth · 2017-08-18T21:52:39Z

Just printing like this would be fine with me:

--------------------
FAILURE
name:       check-fail?
location:   foo.rkt:4:0
actual:     1
expected:   0
actual:     1
expected:   2
--------------------

Anything more complicated I'd defer to a separate package designed with multiple failures in mind.

bennn · 2017-08-19T02:31:14Z

Can simplify 1 more step and only print actual 1 time?
So check-fail prints one actual & multiple expecteds

jackfirth · 2017-09-03T05:13:36Z

This is ready for another review.

Now, the only export of rackunit/meta is check-fail. It accepts a tree of predicates, regexps, and check info values. Here is a sample use and its output:

(define (fail-foo)
  (with-check-info (['foo 1])
    (fail-check "some message from foo")))

(define (has-bar-info? exn)
  (member 'bar (map check-info-name (exn:test:check-stack exn))))

(check-fail (list has-bar-info?
                  #rx"message from bar"
                  (make-check-info 'bar 1))
            fail-foo)

--------------------
FAILURE
name:            check-fail
location:        unsaved-editor:13:0
actual:          (exn:test:check "some message from foo" #<continuation-mark-set> ...)
actual-message:  "some message from foo"
actual-info:
  foo:             1
expected:
  predicate:       #<procedure:has-bar-info?>
  message:         #rx"message from bar"
  info:            (check-info 'bar 1)
--------------------

The check-fail/error form is no longer needed because #75 means check failures evaluated in thunks given to check-exn will raise their exceptions normally and not log failures (hooray!)

See the meta-test.rkt module for a more substantial example of how one would test custom rackunit checks.

bennn

About the error message; I like how the expected looks, but it's hard for me to line it up with the "actual".

Can you change the error message to something like this:

--------------------
FAILURE
name:            check-fail
location:        unsaved-editor:13:0
actual:
  exn:                 (exn:test:check "some message from foo" #<continuation-mark-set> ...)
  message:             "some message from foo"
  info:                ((check-info 'foo 1))
expected:
  exn-predicate:       #<procedure:has-bar-info?>
  message:             #rx"message from bar"
  info:                (check-info 'bar 1)
--------------------

And also sort the expected data, so all predicates, messages, and infos are grouped together?

bennn · 2017-09-08T21:40:33Z

rackunit-doc/rackunit/scribblings/check.scrbl

+amount of logic. Consequently, custom checks can be buggy and should be tested.
+RackUnit provides a few checks explicitly designed for testing the behavior of
+other checks; they allow verifying checks pass and fail when expected or that
+checks add certain information to the check information stack. These bindings


check information stack => @tech{check-info stack}

bennn · 2017-09-08T21:48:39Z

rackunit-doc/rackunit/scribblings/check.scrbl

@@ -1,10 +1,11 @@
 #lang scribble/doc
-@(require "base.rkt")
+@(require (except-in "base.rkt" examples)
+          scribble/example)

 @(require (for-label racket/match))


treeof needs a for-label require ... not sure which one

It's in plot somewhere, but I'm wary of adding a dependency on the plot package's docs to rackunit-doc. Maybe treeof should be moved out of the plot package?

bennn · 2017-09-08T22:04:35Z

rackunit-doc/rackunit/scribblings/check.scrbl

+                     [message string? ""])
+         void?]{
+ Checks that @racket[thunk] raises a check failure and that the failure
+ satisfies @racket[assertion-tree]. The tree is checked in the following manner:


I think the wording here is too low-level (sounds too much like an algorithm to me).

suggestion: change this line & the list to say

satisfies every assertion in assertion-tree. A failure can satisfy an assertion in one of three ways, depending on the type of the assertion:

a failure f satisfies a predicate p if (p f) is a true value

a failure satisfies a regular expression r if r matches the failure's message,

a failure satisfies a check-info structure if the failure's check stack contains the expected value

... okay I don't like what I wrote very much either. Maybe it'd be better to have a deftech for the (treeof ....) contract and define "satisfies" in that?

Anyway, what's important to me is that the 3 cases for an "assertion" have 3 clear meanings. (I don't think the 4-point list & focus on "satisfying a tree" does that.)

I like what you wrote and definitely agree about having three list cases instead of four. I used slightly different wording though.

A deftech seems out of place here, especially since it would only be used in this one check.

bennn · 2017-09-08T22:05:26Z

rackunit-doc/rackunit/scribblings/check.scrbl

+ Checks that @racket[thunk] raises a check failure and that the failure
+ satisfies @racket[assertion-tree]. The tree is checked in the following manner:
+
+ @(itemlist


@itemlist[ .... ]

(same for the examples below)

bennn · 2017-09-08T22:09:49Z

rackunit-lib/rackunit/meta.rkt

+  (define failure (check-raise-value chk-thnk))
+  (unless (exn:test:check? failure)
+    (with-actual failure
+      (fail-check "Check raised error instead of failing")))


replace "failing" with "calling fail-check"?

Because I think:

"failing" is a technical term, it really means to throw an exn:test:check

"failing" isn't described in the docs (anyway, someone writing their first define-check will have trouble with the current error message)

Done, very good idea.

bennn · 2017-09-08T22:10:25Z

rackunit-lib/rackunit/meta.rkt

+(define-simple-macro (with-actual act:expr body:expr ...)
+  (with-check-info* (error-info act) (λ () body ...)))
+
+(define (list/if . vs) (filter values vs))


Can this go in a common place?

I've written this in half a dozen different codebases so really I'd prefer to see it added to racket/list or some list-more package. But I'll put it in rackunit/private/util-list for now (not util, because rackunit/private/check duplicates this and it can't depend on util).

bennn · 2017-09-08T22:12:24Z

rackunit-doc/rackunit/scribblings/check.scrbl

+are provided by @racketmodname[rackunit/meta], not @racketmodname[rackunit].
+
+@defproc[(check-fail [assertion-tree
+                      (treeof (or/c (-> exn:test:check? any/c)


I guess any/c should be any

The predicates have to return one value so I think any/c is appropriate here.

bennn · 2017-09-08T22:12:33Z

rackunit-lib/rackunit/meta.rkt

+                     (procedure-arity-includes? v 1))
+                (regexp? v)
+                (check-info? v))
+      (define ctrct "(or/c (-> any/c boolean?) regexp? check-info?)")


boolean? => any

Changed to any/c (see earlier comment about any vs any/c)

bennn · 2017-09-08T22:15:06Z

rackunit-test/tests/rackunit/meta-test.rkt

+               (λ () (check-fail (list #rx"foo" 'partial-nonsense)
+                                 fail-check)))
+    (check-exn exn:fail:contract?
+               (λ () (check-fail accepts-no-args fail-check)))))


Add some tests with silly lists?

like '(((#rx"hello") ()) #rx"world") ... just non-flat variations on the tests you already have

Done. I think one test that uses all three kinds of assertions in a single silly list ought to be enough; more tests than that seem redundant.

Also add exception and exception message to check info in regex failures instead of only the message.

1.8 was the dynamic-info feature, which was merged before this.

jackfirth · 2017-09-09T06:28:53Z

Sorting and grouping infos in the failure message is now implemented.

bennn

Cool, I like the new docs

bennn · 2017-09-09T16:43:17Z

rackunit-lib/rackunit/meta.rkt

+  (define (exn-info) (make-check-info 'exn (pretty-info raised)))
+  (define (msg-info) (make-check-info 'message (exn-message raised)))
+  (define (info-info)
+    (make-check-info 'info (nested-info (exn:test:check-stack raised))))


I wish the actual and expected info fields looked more similar:

actual: exn: (exn:test:check "some message from foo" #<continuation-mark-set> ...) message: "some message from foo" info: foo: 1 expected: predicate: #<procedure:has-bar-info?> message: #rx"message from bar" info: (check-info 'bar 1)

Because, what if foo isn't a check-info struct, but something else that prints the same way?

I'd like to do (nested-info (map pretty-info (exn:test:check-stack raised))), but that's a contract error.

Do you mean what if foo is a check-info struct who's value is not 1, but something that prints the same such as (string-info "1")? Calling exn:test:check-stack will always return a list of check-info values modulo unreasonable shenanigans.

I didn't know that about exn:test:check-stack.

Okay though, my real complaint is that foo: 1 and (check-info 'bar 1) look too different. I'd rather the first looked like (check-info 'foo 1).

bennn · 2017-09-09T16:46:09Z

rackunit-doc/rackunit/scribblings/check.scrbl

+                       (λ () (check-equal? 'foo 'foo)))]
+
+ Note that a single predicate, regular expression, or @tech{check-info} value is
+ considered a legal tree, as well as the empty list.


I don't think the docs really need this note.

But maybe, add a reminder note not to abuse to tree API? (A check should test "1 thing".)

bennn · 2017-09-09T16:48:20Z

rackunit-lib/rackunit/meta.rkt

+  (with-check-info* (error-info act) (λ () body ...)))
+
+(define (error-info raised)
+  (define (exn-info) (make-check-info 'exn (pretty-info raised)))


I think these thunks are more time/space expensive than just doing (define exn-info (make....

The optimizer should be able to inline them, so I don't think that's worth worrying about.

jackfirth requested a review from bennn July 6, 2017 08:36

bennn reviewed Jul 7, 2017

View reviewed changes

jackfirth changed the title ~~WIP - Add meta checks~~ Add meta checks Jul 13, 2017

jackfirth mentioned this pull request Jul 13, 2017

How to update check-info location? #58

Closed

bennn reviewed Jul 13, 2017

View reviewed changes

bennn mentioned this pull request Jul 30, 2017

check-exn: stop ignore message argument #65

Merged

jackfirth force-pushed the fix-doubletest branch 3 times, most recently from 4f365d9 to 8efcbe5 Compare September 3, 2017 04:49

bennn reviewed Sep 8, 2017

View reviewed changes

Add implementation and tests of meta checks

ea4302b

jackfirth added 13 commits September 8, 2017 21:35

Add failing test for multiple matching infos bug in meta checks

cf773ca

Fix check-fail/info behavior for multiple matching infos

01865d3

Add tests and fix for predicate arity

896596a

Use negate from racket/function

73d97fd

Simplify meta check failure messages

a74c8fe

Also add exception and exception message to check info in regex failures instead of only the message.

Bump version and add history annotations

8d42cd9

Fix grammar

1c1be48

Use examples instead of interaction in docs

e0b8bfb

Rework docs for check-error

d845bb1

Rename check-error to check-fail/error

07a5df9

Move meta checks to separate module

6c8300f

Redesign check-fail to provide a tree-based API

81075ed

Update check-raise-value

fabfad0

jackfirth force-pushed the fix-doubletest branch from e5e7e84 to fabfad0 Compare September 9, 2017 04:36

jackfirth added 10 commits September 8, 2017 21:38

Bump version

70725ac

1.8 was the dynamic-info feature, which was merged before this.

Use brackets, not parens

0ef33c3

Use check info stack tech term

fc439c8

Fix contract error message

052d24c

Move list/if utility to its own module

370edf6

Clarify check-fail message on check error

c7da6ad

Add test case for nontrivial tree input to check-fail

c571f3c

Reword docs slightly for check-fail

80a6f34

Nest all raised value info under 'actual info

48edd6d

Sort expected nested info values by type

925a3a7

Add missing for-label import of rackunit/meta

98d1718

bennn reviewed Sep 9, 2017

View reviewed changes

jackfirth added 2 commits September 10, 2017 23:18

Remove unnecessary prose from check-fail docs

2063233

Add warning against using check-fail with complex trees

a01c91d

jackfirth mentioned this pull request Nov 22, 2017

Add release checklist test: "Rackunit tests" samth/docker-racket-build#33

Open

Add meta checks #59

Are you sure you want to change the base?

Add meta checks #59

Conversation

jackfirth commented Jul 6, 2017 • edited Loading

AlexKnauth commented Jul 6, 2017

jackfirth commented Jul 6, 2017

bennn commented Jul 6, 2017

jackfirth commented Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

bennn Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfirth commented Jul 13, 2017

bennn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennn Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexKnauth commented Jul 13, 2017

jackfirth commented Jul 13, 2017

jackfirth commented Jul 13, 2017 • edited Loading

AlexKnauth commented Jul 13, 2017

rmculpepper commented Jul 13, 2017

jackfirth commented Jul 15, 2017 • edited Loading

bennn commented Aug 1, 2017

jackfirth commented Aug 2, 2017

bennn commented Aug 15, 2017

jackfirth commented Aug 16, 2017

bennn commented Aug 18, 2017

jackfirth commented Aug 18, 2017

bennn commented Aug 19, 2017

jackfirth commented Sep 3, 2017 • edited Loading

bennn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfirth Sep 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfirth Sep 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfirth commented Sep 9, 2017

bennn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfirth commented Jul 6, 2017 •

edited

Loading

jackfirth commented Jul 6, 2017 •

edited

Loading

bennn Jul 13, 2017 •

edited

Loading

bennn Jul 13, 2017 •

edited

Loading

jackfirth commented Jul 13, 2017 •

edited

Loading

jackfirth commented Jul 15, 2017 •

edited

Loading

jackfirth commented Sep 3, 2017 •

edited

Loading

jackfirth Sep 9, 2017 •

edited

Loading

jackfirth Sep 9, 2017 •

edited

Loading