• Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

How to test unknown enum value?

 
Roel De Nijs
Sheriff
Posts: 10040
127
AngularJS Chrome Eclipse IDE Hibernate Java jQuery MySQL Database Spring Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
When trying to increase the test coverage on our project I ran into an interesting issue with enums. Assume the following code:



It's fairly easy to write a test case to verify the calculate method is producing the expected results for addition and subtraction. But if you want complete test coverage for this method you also need a test case for the IllegalArgumentException. How can you achieve such a test? (all testing frameworks are allowed)

I know the implementation of the calculate method is not really object oriented and I should refactor the above code and create a method to the enum to perform the logic. And because we are the owner of the enum, refactoring is exactly what I did in our project But if we were using some enum from a 3rd party (which we can't modify) and we are very keen on 100% test coverage, we have to create such a test.
 
Junilu Lacar
Bartender
Posts: 7466
50
Android Eclipse IDE IntelliJ IDE Java Linux Mac Scala Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
In the case of this example, I'd question the validity of the choice to throw an IllegalArgumentException with a message "operator [op] unknown". Since the op parameter is declared as an Operator, then the compiler guarantees it will be a valid value. The question is whether it's a value that's supported by the calculate() method. If anything, I would change the message to be "operator [op] is not supported". When you do this, the unit test to cover the exception getting thrown is pretty obvious and suddenly makes more sense vs when you have a message that says "operator unknown".
 
Tim Cooke
Sheriff
Pie
Posts: 2972
123
Clojure IntelliJ IDE Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The line where you throw the IllegalArgumentException is non reachable code. Your enum has two values, your switch statement has cases for both values, both cases return from the method. There is no way to get past the switch statement.

I recommend you delete everything after the switch statement. If it's not there you don't need to test it.
 
Roel De Nijs
Sheriff
Posts: 10040
127
AngularJS Chrome Eclipse IDE Hibernate Java jQuery MySQL Database Spring Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Junilu Lacar wrote:If anything, I would change the message to be "operator [op] is not supported".

You are correct. The message of the exception is poorly chosen and this one is a much better one. But that doesn't solve the original problem
 
Roel De Nijs
Sheriff
Posts: 10040
127
AngularJS Chrome Eclipse IDE Hibernate Java jQuery MySQL Database Spring Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Tim Cooke wrote:The line where you throw the IllegalArgumentException is non reachable code. Your enum has two values, your switch statement has cases for both values, both cases return from the method. There is no way to get past the switch statement.

No, it isn't. Without this line the code would not compile, because your method expects an int to be returned.
 
Tim Cooke
Sheriff
Pie
Posts: 2972
123
Clojure IntelliJ IDE Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Well that seems a bit silly. I would consider that a bit of a compiler quirk that it doesn't recognise that you can never get there. In that case it is impossible to get test coverage for that bit of code.

You have two choices:
  • You can just accept it and move on.
  • You can decide that your implementation may not be the best design and take a different, more testable, approach.
  •  
    Junilu Lacar
    Bartender
    Posts: 7466
    50
    Android Eclipse IDE IntelliJ IDE Java Linux Mac Scala Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Roel De Nijs wrote:The message of the exception is poorly chosen and this one is a much better one. But that doesn't solve the original problem


    Pseudocode, assuming JUnit4:



    You might want to create and use another RuntimeException instead, like UnsupportedOperatorException for example, with a field to hold the operator that caused the exception. One thing to note: if you have no unsupported values, then the test will not be able to exercise the throwing of the exception anyway and you won't get 100% coverage. You'll only get 100% coverage if the exception will actually get thrown, which is what I think Tim's point is.
     
    Junilu Lacar
    Bartender
    Posts: 7466
    50
    Android Eclipse IDE IntelliJ IDE Java Linux Mac Scala Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Tim Cooke wrote:In that case it is impossible to get test coverage for that bit of code.[/list]


    Not impossible. Just need to step outside the box a little bit
     
    Stuart A. Burkett
    Ranch Hand
    Posts: 679
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Junilu Lacar wrote:One thing to note: if you have no unsupported values, then the test will not be able to exercise the throwing of the exception anyway and you won't get 100% coverage. You'll only get 100% coverage if the exception will actually get thrown, which is what I think Tim's point is.

    I think that was the point of the original question. The switch handles all the values in the enum so how can the exception throwing be tested as there are no values that won't be handled by the switch.
     
    Junilu Lacar
    Bartender
    Posts: 7466
    50
    Android Eclipse IDE IntelliJ IDE Java Linux Mac Scala Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    If that was the point of the original question, I'd have to agree with Tim in that it's a bit silly to expect to have a way to cover that bit of code with a test if there's no possibility of it ever being executed. Any valid test would have to use values that would actually cause the code under test to throw the exception. Otherwise, the test would just be there as a "safety net" in case a new enum value is added in which case you'd find that you suddenly have 100% coverage.
     
    Tim Cooke
    Sheriff
    Pie
    Posts: 2972
    123
    Clojure IntelliJ IDE Java
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Junilu Lacar wrote:Not impossible. Just need to step outside the box a little bit

    OK. I've stepped outside the box. I'm looking around but I'm not seeing anything. Now I've lost the box..... Can I get a clue?
     
    Junilu Lacar
    Bartender
    Posts: 7466
    50
    Android Eclipse IDE IntelliJ IDE Java Linux Mac Scala Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Tim Cooke wrote:I'm looking around but I'm not seeing anything. Now I've lost the box..... Can I get a clue?

    So you don't think the pseudocode I gave would be a valid test when there are enum values that are not supported by the calculate method?
     
    Tim Cooke
    Sheriff
    Pie
    Posts: 2972
    123
    Clojure IntelliJ IDE Java
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Yes. But I don't see how you can get unsupported enum values into the calculate method without adding them to the production code.
     
    Junilu Lacar
    Bartender
    Posts: 7466
    50
    Android Eclipse IDE IntelliJ IDE Java Linux Mac Scala Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Tim Cooke wrote:Yes. But I don't see how you can get unsupported enum values into the calculate method without adding them to the production code.


    I'm not sure I follow your line of thinking here. I'm assuming that the given code snippet is just an example similar to the actual code or an abbreviation of the actual code. Also assuming that the actual enumerated type has more values and that some values are not supported. Otherwise, there really is no basis for expecting to get 100% test coverage as I mentioned before.
     
    Roel De Nijs
    Sheriff
    Posts: 10040
    127
    AngularJS Chrome Eclipse IDE Hibernate Java jQuery MySQL Database Spring Tomcat Server
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Stuart A. Burkett wrote:I think that was the point of the original question. The switch handles all the values in the enum so how can the exception throwing be tested as there are no values that won't be handled by the switch.

    That was indeed the original question!

    Junilu Lacar wrote:I'd have to agree with Tim

    I also totally agree with Tim: using switch statement is in most cases not the best design, so it can be refactored to a more testable approach. And that's what i did in the current project. But some curiosity kicked in: what to do if I could not refactor this code, because e.g. the enum was from a 3rd party library and I can't change it? With mocking frameworks you can mock static methods and private methods, so I was just wondering if it was possible to mock some bogus enum constant and use that one to cover this line. If this test can be added, then the build will fail if a new enum constant was added (in a new version of this 3rd party library) but not covered in our method. So the "broken" method will not make it to production before it was fixed.
     
    Tim Cooke
    Sheriff
    Pie
    Posts: 2972
    123
    Clojure IntelliJ IDE Java
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Right ok. I wasn't making any assumptions and was taking the op's code as is with two enum values, both handled in the switch.

    I see right enough that if the prod enum contained unsupported values then testing would be trivial.
     
    Roel De Nijs
    Sheriff
    Posts: 10040
    127
    AngularJS Chrome Eclipse IDE Hibernate Java jQuery MySQL Database Spring Tomcat Server
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Junilu Lacar wrote:Also assuming that the actual enumerated type has more values and that some values are not supported.

    My mistake: I mislead you completely with the post about the exception message. I didn't mean "unsupported" as in "I have 5 enum values and this method handles 2, so I want an exception when invoked with one of the other 3", but "unsupported" as in "I have 5 enum values, all covered in this method. But I want to throw an exception if a 6th value is added to the enum and I forget to handle this new enum value in the method". That's why I used "unknown" in the original post. Sorry for that!
     
    Junilu Lacar
    Bartender
    Posts: 7466
    50
    Android Eclipse IDE IntelliJ IDE Java Linux Mac Scala Spring Ubuntu
    • Likes 1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I think you should modify your approach. Instead of expecting the test for the exception being thrown to fail, you should write another test that explicitly checks for the number of values that your current code expects there to be:



    This test will fail when the number of enumerated values changes. At that point, you have to re-examine your design and existing tests and change the hard-coded value as necessary so that future changes can be flagged.
     
    Paul Clapham
    Sheriff
    Posts: 21107
    32
    Eclipse IDE Firefox Browser MySQL Database
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I see there's a lot of people saying what I originally thought but never got around to posting: if you have code which can't be tested, then perhaps there's a design problem.
     
    Junilu Lacar
    Bartender
    Posts: 7466
    50
    Android Eclipse IDE IntelliJ IDE Java Linux Mac Scala Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    In this case, Paul, I don't think it's about design and untestable code. The goal is to have a way to detect that certain assumptions have been invalidated or need to be adjusted. If a new enumerated value is added, then there's a possibility that test/production code should be changed. The first test that I suggested would cover the exception being thrown for unsupported values. The "flagger" test would tell the developer that a review of the current design and code should be done.
     
    Roel De Nijs
    Sheriff
    Posts: 10040
    127
    AngularJS Chrome Eclipse IDE Hibernate Java jQuery MySQL Database Spring Tomcat Server
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Junilu Lacar wrote:Just need to step outside the box a little bit.

    That's what I did when I started experimenting with the testing and mocking frameworks I know And it really helped! I created a test case using the latest versions of JUnit, EasyMock and PowerMock which gives 100% test coverage for the Calculator class from the original post.

    Here is the code:
     
    Junilu Lacar
    Bartender
    Posts: 7466
    50
    Android Eclipse IDE IntelliJ IDE Java Linux Mac Scala Spring Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    A few comments:
    1. I prefer to write tests that check for one thing -- your calculate test asserts three different things. I would break this up into three separate tests.
    2. I don't see how this test will flag the fact that there is a new enum value if the enumerated type changes. To verify this, add a new operator enum value in the real code and run the test again. I'm pretty sure it will still pass.
    3. I don't think the 100% coverage goal is an absolute need for the example you gave. It seems to me that it's more important to get a reminder that you need to check your assumptions and implementation again when a change to the enumerated type is detected. This is what the flagger test that I suggested would do.
     
    Roel De Nijs
    Sheriff
    Posts: 10040
    127
    AngularJS Chrome Eclipse IDE Hibernate Java jQuery MySQL Database Spring Tomcat Server
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Junilu Lacar wrote:I prefer to write tests that check for one thing -- your calculate test asserts three different things. I would break this up into three separate tests.

    If you want to verify the Operator class I don't see any option other than grouping all asserts in 1 test. I tried the AfterClass annotation, but that didn't work well.

    Junilu Lacar wrote:I don't see how this test will flag the fact that there is a new enum value if the enumerated type changes. To verify this, add a new operator enum value in the real code and run the test again. I'm pretty sure it will still pass.

    It will only pass if you expect the IllegalArgumentException to be thrown, otherwise your test will fail. Demonstrated in the test case at the bottom of this post.

    Junilu Lacar wrote:I don't think the 100% coverage goal is an absolute need for the example you gave. It seems to me that it's more important to get a reminder that you need to check your assumptions and implementation again when a change to the enumerated type is detected. This is what the flagger test that I suggested would do.

    It's indeed more important to get a reminder, how it's achieved is less important. But if you are using some code quality tool like Sonar, this test case gives you 100% test coverage and that's what we try to achieve for every project (and that's why I started wondering about this scenario in the 1st time)

    Added a multiply operator:


    Slightly updated the test case (to be completely independent on how much values the enum has + each assert has its own test). This test runs successfully.
     
    • Post Reply
    • Bookmark Topic Watch Topic
    • New Topic