Clojure

Syntactically broken clojure.test/are tests succeed

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: Release 1.3
  • Fix Version/s: Release 1.4
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code
  • Approval:
    Ok

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.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Andy Fingerhut added a comment -

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?

Show
Andy Fingerhut added a comment - 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?
Hide
Permalink
Tassilo Horn added a comment -

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.

Show
Tassilo Horn added a comment - 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.
Hide
Permalink
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Permalink
Tassilo Horn added a comment -

I'm working on it...

Show
Tassilo Horn added a comment - I'm working on it...
Hide
Permalink
Tassilo Horn added a comment -

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?!?
Show
Tassilo Horn added a comment - 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?!?
Tassilo Horn made changes -
Field Original Value New Value
Attachment 0001-Fix-CLJ-931-Syntactically-broken-clojure.test-are-te.patch [ 10919 ]
Tassilo Horn made changes -
Attachment 0001-Throw-exception-if-are-s-args-don-t-match-its-argv.patch [ 10914 ]
Tassilo Horn made changes -
Attachment 0001-Fix-CLJ-931-Syntactically-broken-clojure.test-are-te.patch [ 10919 ]
Hide
Permalink
Tassilo Horn added a comment -

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

Show
Tassilo Horn added a comment - Another minor update. Now (are [] true) is allowed (but meaningless). In the previous patch, you got a "divide by zero" in this case.
Tassilo Horn made changes -
Attachment 0001-Fix-CLJ-931-Syntactically-broken-clojure.test-are-te.patch [ 10920 ]
Hide
Permalink
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Permalink
Stuart Sierra added a comment -

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

Show
Stuart Sierra added a comment - I can't make the test work either. The patch looks good. Just remove the broken test instead of leaving it commented out.
Hide
Permalink
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Andy Fingerhut made changes -
Attachment clj-931-error-on-bad-are-syntax-patch2.txt [ 10989 ]
Hide
Permalink
Stuart Sierra added a comment -

Screened.

Show
Stuart Sierra added a comment - Screened.
Stuart Sierra made changes -
Waiting On richhickey
Approval Screened [ 10004 ]
Priority Major [ 3 ] Minor [ 4 ]
Stuart Sierra made changes -
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:

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

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

The attached patch makes clojure.test/are take care that the number of values is divisible by the number of variables declared in the vector and throw an exception if that's not the case.

Stu, I'm still confused about the workflow. Should I've been adding this report to some Fix Version(s), and if so, which? 		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:

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

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.
Rich Hickey made changes -
Approval Screened [ 10004 ] Ok [ 10007 ]
Fix Version/s Release 1.4 [ 10040 ]
Stuart Halloway made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Closed [ 6 ]

People

Vote (1)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.