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?!?
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.
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.
Hide
Permalink
Stuart Sierra added a comment -

Screened.

Show
Stuart Sierra added a comment - Screened.

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.