• P (unregistered)

    Teddy's unit test code is unbearable as usual. Kudos for Rick and Linda to bear with him again.

  • EwgB (unregistered)

    Let me point out an edge case where enum values order does matter. When you write enum values into a database with their numeric values, if you change the order, the semantic changes. See for example https://tomee.apache.org/examples-trunk/jpa-enumerated/ (where it is listed as a disadvantage of using this approach, so don't @ me).

  • (nodebb)

    Assert.AreEqual(1.ToString(), "1");

  • (nodebb)

    Better yet: Assert.AreEqual(1, 1);

  • P (unregistered) in reply to Mr. TA

    Might as well just use Assert.Pass().

  • Remcoder (unregistered)

    You missed the possible ArrayIndexOutOfBoundsException.

  • Rick Poleshuck (google)

    I am sorry to disagree with Remy, but Teddy does understand enums. He just doesn't understand the purpose of unit testing. This is a much bigger problem which is so prevalent that it doesn't earn a WTF in my opinion.

  • Pedant (unregistered)

    "I suppose this makes their code coverage metrics look better, at least."

    Not true. The test doesn't cover any of the code, so code coverage metrics would be unaffected with or without this test.

  • Lothar (unregistered)

    Have to agree with EwgB. Java enum entries have an ordinal() that returns the index in the enum-list. If you e.g. change the order in an enum or remove an entry the returned values of ordinal change as well. If these numeric values are used (e.g. database but serialization comes to mind as well) a change in the enum can break a lot of stuff.

    So having a Unit-test to ensure that an enum doesn't change in such a way is actually a good thing and no WTF at all. At least with Java enums.

  • Brian (unregistered)

    Ok, I'll bite. A case could be made for checking the values of an enum, such as when they correspond to some external interface. I have exactly that scenario in my codebase: we're maintaining a legacy client-server app that uses raw socket-based communications with numeric codes to identify the message types. In such a case, doing anything to alter the enum values is a breaking change that wouldn't be caught at compile-time.

    Checking just the names, though, as this snippet does, is entirely pointless.

  • doubting_poster (unregistered) in reply to Brian

    agreed. If there's a hard requirement on an enum to match some other source of data, then adding a unittest that checks nobody changed it makes sense. Of course, this test would still be wrong for all the other reasons.

  • THeCPUWizard (unregistered)

    Yes, the following is c# and not Java....

    public enum FriendlyColorsEnum { [Description("Blanched Almond Color")] BlanchedAlmond = 1, [Description("Dark Sea Green Color")] DarkSeaGreen = 2, [Description("Deep Sky Blue Color")] DeepSkyBlue = 3, [Description("Rosy Brown Color")] RosyBrown = 4 }

  • Rick (unregistered) in reply to Remcoder

    "You missed the possible ArrayIndexOutOfBoundsException."
    Good catch Remcoder! Yes, if someone removed an entry from the enum so the hard-coded list was longer than the enum, instead of the test failing you'd an out of bounds exception.

  • (nodebb)

    Echoing that second comment, there are other instances besides database storage where that effect would come into play. For example, some API takes integers and you've defined an enumerator to help remember what the integers represent. Examples of that are all over [http://pinvoke.net], something that might not be readily apparent if you don't leave the BCL. A corner case that sounds absurd, but I can't dismiss it due to all of the absurdity I've seen firsthand, is that some fool out there enforces a code "standard" that doesn't allow explicitly assigning values to enumerators. In any case where you need to integers behind the enumerator, changing its order would be a bug that wouldn't be caught by the compiler or any scanning tools that I know of.

  • Lothar (unregistered) in reply to Brian

    Checking just the names, though, as this snippet does, is entirely pointless. It implicitly checks the ordinals as well by checking the order of the values.

    But while we're at it, let's go through Rick's suggestions:

    • the pointlessness of unit tests of enums (or the old sweats pointing out the couple of valid edge cases)

    As pointed out, it might not be pointless if the enum is used "outside the code as well", e.g. their ordinal-values in databases or the value names in JSON or XML or any other case where ENUM.values()[ordinalvalue] or ENUM.valueOf(ENUM.class, "ENUMVAL") is used, both of which throw exceptions or return wrong values later on. Serialization (be it JSON, XML or old style DataOut.writeObject) is not an edge case.

    • the reversed names of actual vs expected results

    The names are correct if you look at the assertEquals-method. They are initialized with the wrong values.

    • the useless message in assertEquals

    It's not useless because it shows up e.g in the HTML JUnit-Report that pops out of the production process, so you as a human get an idea what failed instead of having to go to the source and find the line being reported in the stacktrace.

    • the fact that the test requires the enum to be in a certain order to pass

    See above. If other parts of the code use the ordinal value and use that to create the enum-value at a later point of time, that requires the order to be fixed. Just imagine your fancy Transaction Isolation Level Configuration Enum changes its order and your database connection changes from Read committed to Read uncommitted

    • the fact that the test will still pass if extra values are added to the enum

    That one is true and should be fixed (not by entirely deleting the test, see above, why)

    • the oddly double-up class name (yes it really is named in the format testEnumName_EnumNameExists)

    No, it's the name of the method. I've seen worse.

    All in all I don't actually see, why the code is a WTF more the test-deleting people

  • ooOOooGa (unregistered)

    If the enum is being used outside of the code in a way where the ordinal value or name is important, then it really isn't an enum. It is a map <integer, string>.

    Granted, the distinction is rather subtle since enum is typically represented in code as a map <integer, string>.

    But remember, the concept of an enum is not an integer. That is just how we normally represent them.

    So yes. If this is actually an enum, then there is zero point in verifying that the order of items, the number of items, their ordinal value, or their names remain constant.

  • (nodebb) in reply to EwgB

    It's nice how they summarise the disadvantage: "If the order of the enum values is significant to your code, avoid EnumType.ORDINAL"

    Don't use ORDINALs if order is significant.

  • Lothar (unregistered) in reply to Watson

    In other words, don't use switch-statements with the enum-values in the cases unless that class is ensured to be compiled together with the change to the enum. That's another of these "edge-cases"...

  • Twither (unregistered) in reply to doubting_poster

    If there's a hard requirement on an enum to match some other source of data, then adding a unittest that checks nobody changed it makes sense.

    How does that make sense? Presumably, if you have an external integer value that needs to be converted to an internal type, you need to have a mapping for that somewhere in the code. Somewhere that can be updated if and when the spec changes. The enum is as good a place to do that as any other, I suppose. But if you're writing unit tests to make sure the enum doesn't change, now you have to update two things when the spec changes. And if someone adds entries to the enum, the test still passes and therefore won't be updated. Now when someone changes the enum, maybe the test will fail, maybe not.

    On the other hand, when I think about WinForm's System.Drawing.RotateFlipType... the enum is the mathematical group D_4, and the enum values are chosen to make composition of the transforms easy. Changing their values would break the code without the compiler noticing. Nevertheless, the unit test should verify the composition logic, not the enum values.

  • Gerry (unregistered)

    I see an (over) abundance of caution on Teddy's part. As stated by the other very experienced programmers ("old sweats"), there are many ways that changing the order of an enum can break code (enumVal < whatWas4thValue).

    Possibly Teddy has had to deal with the consequences of less experienced programmers (dumb, over confident newbies) add new enums into the middle, or changing the order. He then developed a strategy to prevent this from happening in the future.

  • Irish Girl (unregistered)

    If a developer adds a new entry, it probably belongs there.

    enum Bool { True, False, FileNotFound };

  • David Mårtensson (unregistered) in reply to EwgB

    Which is a WTF all by it self as its more or less designed to fail some time in the future.

  • MaxiTB (unregistered)

    Checking names makes sense actually.

    For public contracts it is important if the value is serialized by name.

    Sure, there are better ways to achieve contract validations and I am not even commenting on the implemenation or awful AAA imitation attempt, but at least there is a unit test that gives the next dev a reason to check whats up.

  • I dunno LOL ¯\(°_o)/¯ (unregistered)

    Every time Teddy is mentioned in this, I can't help but think of Teddy from Bob's Burgers. It's just the clueless sort of thing he'd do... if he was writing code.

Leave a comment on “Enumerating Your Failures”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article