How do I convince some one that test should do assertion (not assertions) and not the helper methods

Question

Joined a new employer and came across a new style of writing tests.

@Test()
public testMethodWhichDoesNotDoAnyAssertion() {
    LoginPage loginPage = signUpPage.doLogin("username","password");
    oneMoreCommonMethodCalledHere()
    anotherCommonMethodCalledHere()
}

public void doLogin(String userName, String password) {
     //login here
     Assert.assertTrue("Login Successful")
}

public void oneMoreCommonMethodCalledHere() {
     //Some more operations here.
     Assert.assertTrue("This also succeeded")
}

public void anotherCommonMethodCalledHere() {
     //Some more operations here.
     Assert.assertTrue("Even this succeeded!!! Your code is awesome!!!")

So far I have been doing assertions in tests and not in the methods which are invoked from test method. The problems I have with approach are multiple -

There are two many assertions happening in one method, though indirectly and it defeats the idea of one responsibility per test

There are times when I want to do one assertion in my test method while testing for a work flow. And many of the helper methods which would be called would assert things and might even fail which would hamper work flow test.

Now thing I have heard in favour of this approach - It is easy for any one to just plug in helper method in a test while not worrying about the assertions which should be carried out for a scenario, as helper method takes care of it.

Comments?

Answer 1

Below I assume the helper methods you show are test helpers, not part of the production code (this is not clear from your post, as @Martin already noted).

There are two many assertions happening in one method, though indirectly and it defeats the idea of one responsibility per test

I personally don't take the "one assertion per test" rule (or might I call it dogma). I prefer to be pragmatic, and in my experience such a rule would hinder development. My rule is "one scenario per test" - and that single scenario may well require multiple assertions to verify the results.

However, I agree that having assertions intermixed with calls to the code under test is somewhat confusing. I prefer the Arrange-Act-Assert style as it makes my tests clearer - not only for others, but for the future myself too. Nevertheless, I do often put one or more assertions into dedicated helper methods - just in this case, those helper methods are meant only to assert results, not to act, and their name reflects this.

Now, at any rate, being the new guy in the team, you better understand and get used to the already established practices and conventions of the team, in order to become a useful team member (or start looking for a better workplace if you really really really feel you can't take this). After that, you can gradually start infusing your own ideas and thoughts, even criticism, into the thought process of the team. But trying to change team conventions up front, without proving your worth and gaining respect of your colleagues first, is going to be a futile and frustrating experience for you.

There are times when I want to do one assertion in my test method while testing for a work flow. And many of the helper methods which would be called would assert things and might even fail which would hamper work flow test.

Two notes:

• Why do the assertions fail? My first instinct is that in a good test they shouldn't, and removing the failing assertions may actually hide a real problem.

• What stops you from refactoring the helper methods? E.g.

  public void doLoginWithoutAssert(String userName, String password) {
       //login here
  }
  
  public void doLogin(String userName, String password) {
       doLoginWithoutAssert(userName, password);
       AssertTrue("Login Successful");
  }
  

  This way you can call the assertion-free helper while your colleagues keep calling their original helpers, and everyone's happy.

Answer 2

The problem with that approach is that contrary to real "asserts" (I suppose we're talking Java here), the code is not removed by the compiler when compiling for a production release. It stays there. Sure, it's just an "if" that takes 0.0001 second to complete and performance will not be impacted much, but it seems to me that it pollutes the code.

I understand that your test case code is smaller, but what I always read everywhere is that test code should not be mixed up with business code. Moreover, your production code has most probably a dependency on a testing framework. However, your fellow colleague may perhaps start a new trend ? ;-)

Also, at first, I thought your colleague was using Spring Framework's Assert which is used to check your method parameters primarily. If he likes the "assert" keyword maybe you could convince him to use that instead, though it has nothing to do with testing :-)

Answer 3

Ask the question : "What is the responsibility of these helper methods?" Is it that the helper method should do some operation and return some value indicating that the operation was success, or is it that the helper method does some operation and if it not successful the should out loud in a speaker (rather than whispering the result back to the caller) about the failure of the operation and hence making them dependent on the speaker to which they can shout.

Answer 4

Seems that the tests' assertions become a bit convoluted as the common methods are called several times during test, the assertions will be done several times at once. Usually you want one assertion per test.

From a maintainability point of view I guess the test code is okay, though I personally prefer refactor tests so that common setup code (that are not in the @Before method) are extracted to their own methods. These extracted methods I usually prefix with when as method is named whenLoginIsPerformed(…) (terminology taken from the Mockito mocking framework). What you name it doesn't really matter if you want to go by the AAA as a mnemonic, you could prefix it with arrange such as method is named arrangeLoginIsPerformed(…).