<< Back to previous view

[CLJ-931] Syntactically broken clojure.test/are tests succeed Created: 16/Feb/12  Updated: 23/Mar/12  Resolved: 23/Mar/12

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3
Fix Version/s: Release 1.4

Type: Defect Priority: Minor
Reporter: Tassilo Horn Assignee: Stuart Sierra
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 0001-Fix-CLJ-931-Syntactically-broken-clojure.test-are-te.patch     Text File clj-931-error-on-bad-are-syntax-patch2.txt    
Patch: Code
Approval: Ok
Waiting On: Rich Hickey

 Description   

While clojure.test/are is a very useful macro, it has one major flaw. If the assertion is syntactically incorrect, the test succeeds. Take this testcase:

(deftest broken-test
  (are [a b c] (= a b c)
       1 1))

See the error? The are form takes three values, but I have provided only two. The test simply passes.

Latest patch checks the number of arguments to are and throws an exception if they don't match.

 Comments   
Comment by Andy Fingerhut [ 16/Feb/12 11:36 AM ]

Is this the same issue as CLJ-806? If so, it might be nice to close one with a comment that it is a duplicate of the other. Given CLJ-806 has no patch, and this one does, perhaps preferable to close CLJ-806?

Comment by Tassilo Horn [ 16/Feb/12 11:46 AM ]

John, you are right. I didn't find it searching for "test are" earlier today; now I do.

I did as you've suggested: closed CLJ-806 in favour of this one.

Comment by Andy Fingerhut [ 17/Feb/12 2:27 AM ]

I've tested this patch by hand, and it seems to work fine, and the code looks correct. I have tried several ways to add a unit test to verify this fix, but haven't found the right way to do it, if there is one.

Comment by Tassilo Horn [ 17/Feb/12 2:58 AM ]

I'm working on it...

Comment by Tassilo Horn [ 17/Feb/12 4:10 AM ]

This patch obsoletes the former on (I'm gonna delete the old one). It includes:

  • an extended fix that catches also the zero-arg given case
  • a fix for a testcase in test/clojure/test_clojure/java_interop.clj which only passed because of the bug this issue is all about
  • a currently #_-disabled test case for testing `are'. It is disabled, because it fails when run with "ant test". However, it works just fine when run inside some REPL. I presume that's because ant runs inside its own JVM instance and thus makes catching exceptions from macroexpand/eval forms impossible?!?
Comment by Tassilo Horn [ 17/Feb/12 4:36 AM ]

Another minor update. Now (are [] true) is allowed (but meaningless). In the previous patch, you got a "divide by zero" in this case.

Comment by Andy Fingerhut [ 09/Mar/12 9:29 AM ]

Patch 0001-Fix-CLJ-931-Syntactically-broken-clojure.test-are-te.patch applies, builds, and tests cleanly as of March 9, 2012 latest master. One author Tassilo has signed CA.

Comment by Stuart Sierra [ 09/Mar/12 10:11 AM ]

I can't make the test work either. The patch looks good. Just remove the broken test instead of leaving it commented out.

Comment by Andy Fingerhut [ 09/Mar/12 10:24 AM ]

clj-931-error-on-bad-are-syntax-patch2.txt same as previous one, except removes commented out test. Applies, builds, and tests cleanly to latest master.

Comment by Stuart Sierra [ 09/Mar/12 10:37 AM ]

Screened.

Generated at Fri May 24 08:50:59 CDT 2013 using JIRA 4.4#649-r158309.