redding / assert

Assertion style testing framework.
https://github.com/redding/assert
MIT License
10 stars 1 forks source link

halt test when assertion fails and count tests not assertions #68

Closed jbgo closed 13 years ago

jbgo commented 13 years ago

I have 4 tests, but assert reports 19 test results - the total number of assertions. Also, when an assertion fails the test continues.

Loaded suite (4 tests)
Running tests in random order, seeded with: "18861"
..FF....FF....FF..F

[...test results were here...]

19 test results: 12 passed and 7 failed

The expected behavior when an assertion fails is to halt the test. Here is a pathological case of how things could go very wrong if a test is not halted when the first assertion fails:

some_class.rb

class SomeClass
  attr_reader :x

  def init(x=1)
    @x = x
  end

  def run
    unless @x > 1
      delete_everything!
    end
  end

  def delete_everything!
     `rm -rf /` # just go with it
  end
end

test/some_class_test.rb

require 'assert'

class SomeClassTest < Assert::Context
  def test_database_not_deleted
     s = SomeClass.new
     assert_equal 2, s.x # assertion fails
     s.run # oops!
     assert File.exists?('/')
  end
end
jcredding commented 13 years ago

Interesting thoughts.

The showing the number of tests instead of results is kinda a personal preference thing, only thing I could think of is to add to the summary the number of tests and maybe tweak the sentence:

19 results: 12 passed and 7 failed in 4 tests

That might add some clarity that each assertion is considered a result. You can always write your own view as well with assert-view.

As far as stopping on assertion failure, I'm not sure, I can clearly see your point and how if you were depending on this behavior it could burn you. I can't remember the exact reason why @kelredd chose not to have it stop, but you are losing being able to see that multiple assertions failed and fixing them all at the same time. Otherwise you would have to run the test for every assertion, fixing one at a time.

@kelredd you might think about designing this as a 'way' to run your tests, like running them in random order. I really think this is kinda a personal preference to how you want your tests run, personally I'm fine with it not stopping, but I can see how others would prefer it behave more like test-unit and stopping.

jbgo commented 13 years ago

If you plan on keeping the current behavior, you should use "19 assertions: 12 passed and 7 failed in 4 tests". These are assertions, not results, not tests. Let's be specific.

As far as continuing to run a test when an assertion fails, I will buy you lunch this Friday if you can find me another testing framework that does this. This violates the entire concept of an assertion. When assertions are used outside the context of testing, such as in actual production code (i.e. in your business logic), the default behavior in most languages with assertions built in is to raise an exception if the assertion fails. Maybe if you used assumptions instead of assertions I wouldn't be so opposed to the idea. But if an assertion failsin my program, test or not, I want my program to halt rather than continuing on in an undefined state.

At the very least, could you provide an option to toggle between normal assertion behavior and assert assertion behavior?

On Wed, Sep 14, 2011 at 2:26 PM, Collin Redding reply@reply.github.com wrote:

Interesting thoughts.

The showing the number of tests instead of results is kinda a personal preference thing, only thing I could think of is to add to the summary the number of tests and maybe tweak the sentence:

   19 results: 12 passed and 7 failed in 4 tests

That might add some clarity that each assertion is considered a result. You can always write your own view as well with assert-view.

As far as stopping on assertion failure, I'm not sure, I can clearly see your point and how if you were depending on this behavior it could burn you. I can't remember the exact reason why @kelredd chose not to have it stop, but you are losing being able to see that multiple assertions failed and fixing them all at the same time. Otherwise you would have to run the test for every assertion, fixing one at a time.

@kelredd you might think about designing this as a 'way' to run your tests, like running them in random order. I really think this is kinda a personal preference to how you want your tests run, personally I'm fine with it not stopping, but I can see how others would prefer it behave more like test-unit and stopping.

Reply to this email directly or view it on GitHub: https://github.com/teaminsight/assert/issues/68#issuecomment-2097032

tpett commented 13 years ago

This unconventional use of the term assert bothers me too. I do actually prefer the result of seeing my individual assertions as individual results in most cases. There are times when I could probably split a single test into 4 tests, but it is just simpler to have 4 assert statements. It is nice to see the pass/fail status of each of them individually. Of course there could also be situations where assertions are dependent on one another and if one fails the next one may report an incorrect pass/fail status.

I do agree with Jordan though in that if we are saying assert it needs to halt execution if the assertion failed. It is the definition of the term. I wouldn't be opposed to having parallel methods though called something like check or test that functioned like the current assert ones do.

Also, whatever we decide to do with the method name we should match the nomenclature of the test output to that of the suite. If the suite is 10 tests 20 assertions and we are showing the assertions in the output it should say 20 assertions not 20 tests.

http://en.wikipedia.org/wiki/Assertion_(computing)

jbgo commented 13 years ago

I would still prefer an option to toggle how assertions behave when running tests as opposed to using different methods. Perhaps we could provide a @halt_when_assertions_fail@ boolean option.

My reasoning is that if for some reason I wanted to change how my tests behave, perhaps because I'm having trouble figure out why a particular test is failing, it's a lot easier to toggle an option on the command line or in a file somewhere than rename all of my assert/check methods.

kellyredding commented 13 years ago

I will open an issue on assert-view to change the default result statement to something more like 23 assertions: 20 passed and 3 failed in 17 tests. However, the problem I have with calling it 'assertions' is that its not assertions - its results. The design goes like this:

So that's why I chose to, by default, call them 'test results' (not 'assertions', not 'tests') in the summary output. Thoughts on this before I open the issue on assert-view?

kellyredding commented 13 years ago

(notes to self) Prevailing opinion is to go with convention and have failed assertions halt test execution by default. Will also provide a flag/option to override this and have failed assertions not halt execution.

I've opened up an issue on assert-view to handle changing the results statement on the DefaultView (https://github.com/teaminsight/assert-view/issues/5).