Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Example of cleanup in a test #45

Open
wants to merge 26 commits into from

2 participants

@elefevre

I mentioned to Andrew that I feel that comments are not necessary in tests and also in internal classes.

This is an example of what I mean. You'll see that I went from ~1300 lines down to less than 1000.
If you are happy with that, I plan to simplify this test more (and other tests, too). This is the low-hanging fruit, though.

Let me know what you think

Eric

@ALRubinger

Partly because it makes the code using it easier to read (being in italic). Also because it makes it clearer that it is a utility method, and is not related to the production code. It puts it to the same level as the other utilities methods that are statically imported, such as assertThat().

@ALRubinger

return ArchivePaths.create(filename); // More appropriate, to go through the API and not link to BasicPath (internals)

agreed (although I don't think using internals is as much as a problem in the tests as it would be in the production code)

@ALRubinger
Owner

Super.

@ALRubinger
Owner

Worthy issue. I'm all for cleanup of the tests, so long as we're not changing test logic or losing vital doc data by being too aggresive w/ our cleanup. I made a new sample comments on a couple of these commits, but didn't yet review the full thing. First:

Can we untangle the SHRINKWRAP-353 stuff from here?

Then I might look to squash this down to one commit and review the whole thing. Feel free to open a JIRA and assign to yourself in order to track this.

@ALRubinger

Why are we removing class file docs? This needs to stay (though the @version line does not).

@ALRubinger

I rather like these separators; I find them helpful when scanning files to get to the right section quickly. If you would like to debate their validity, see if you can find anyone in the ShrinkWrap Dev forums to back you up in removing them. With some consensus against, we can remove them, but honestly I'd prefer stick w/ the status quo here.

@ALRubinger

Clearly in this case this could be an excessive doc, but in general I like to note what everything is used for if it's not blaringly obvious. IMO the risk of being too explicit is far less than the risk of not writing enough line-level docs. So thematically you'll see in everything I write a level of verbose documentation that's way higher than average.

@ALRubinger
Owner

The problem w/ static imports is IDE support; nothing more, nothing less. Once in place they're nice, but a mass "Organize Imports" can mess things up when they're iniitally brought in.

I'm confused... Are you saying that they work poorly in older IDEs? I cannot remember having problems requesting an Organize Import, at least with Eclipse.

@ALRubinger
Owner

In the quest to reduce line count by removing the reference to "Archive archive" in favor of "getArchive" directly, we've actually made things less testable w/ a debugger, as we've no reference to inspect. So I don't see the benefit of this particular patch.

Archive is a field in this test. What's hard about checking its value in the debugger?
What I don't like is the archive variable in every single method. That's overkill and does increase the line count too much.
My preferred style would be to refer to the archive field, but I refrained from that on the basis that sub-tests refer to getArchive() already.
Now that I think about it, I'd rather make the archive field protected (not private), initialize it in an @Before method and remove the getArchive() method.
Thoughts?

@ALRubinger
Owner

Cleaner, but more incorrect. The explicit catch blocks test that IllegalArgumentException is received in only that scope, not anywhere else in the method. By catching IAE at in @Test we could get false positives on test execution if IAE is received from another line than the one intended to be tested.

That's true in theory, but I think this is well worth the tradeoff.
If we initialized the archive field in an @Before method, that problem would be alleviated somewhat.

@ALRubinger
Owner

+1, good centralization and DRY.

@ALRubinger
Owner

Good, but in this patch we've dropped the assertion error messages while converting to assertArrayEquals.

Well, you I like it that way now ;-)

@ALRubinger
Owner

Veto. :P "final" in my estimation should be a default. If I want a variable to be reassignable, I'll make it such.

Maybe it should be default, but it's not. I prefer to err on the side of more readability (as you must be aware now). This is particularly true in tests were the code does not need to be as solid.
That said, I'd remove it in production code too.

@ALRubinger
Owner

Messages from assertions aren't about clearing up the intent in the source, they're about reporting some information about the assertion failure. So these messages show up in the logs; if you remove them you lose that context.

I agree that you lose the context in the logs. However, I do not believe that they are worth the trouble of writing, maintaining and reading (when maintaining the code). Personally, when I get an error in the command line or Jenkins, I rarely go check the logs. I immediately open Eclipse. I should also add that I tend to run my tests under Eclipse very often, which reduces the need to check out the logs even more.
(on a related topic, I don't like the @After println in the test -- a real test should not write anything; I haven't had the heart to delete it yet)

A directly related (I think) issue is that the assertions being used are not explicit enough. assertNotNull and assertTrue are especially guilty of that. In most cases, we can probably replace them with assertThat() clauses. In some cases, we'll have to create our own Hamcrest matchers to get much clearer error.
Typically, assertTrue(getArchive().contains(PATH_1)) would be replaced by assertThat(getArchive(), contains(PATH_1)).

So, that's also something I'd like to do. In the meantime, I believe that the current messages are not pulling their weight. And surely, as a beginner, I would have been the person to benefit most from them.

Hm... I really should write a blog post about all this some time.

@ALRubinger

Why public? package-level might do. Keep in mind we don't want to encourage test interdependencies where they need not exist.

I mostly replaced references to NAME_TEST_PROPERTIES made by other tests. I was hoping to eliminate references to NAME_TEST_PROPERTIES (and make it private), but that didn't seem possible.
The cleaner solution is to copy NAME_TEST_PROPERTIES in those tests, I think.

@ALRubinger
Owner

+1 on all the ASSET_1 stuff

@ALRubinger
Owner

What's your reasoning behind investing in replacing the inlines?

Essentially, it makes the code more "fluent". You can read code like telling a story to yourself and you don't have to try remember what a variable stands for.
What I sometimes do, also, is rename a variable so that the name become the same as the value itself. eg. String eric = "eric". But it's rarely worth it, except for constants.

It is also closer to the way the code is actually being used in production: you don't usually don't use the same pointers to add data and request data.

Of course, it also reduces the number of lines, which is nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 14, 2011
  1. @elefevre
  2. @elefevre

    Minor comment clean up

    elefevre authored
  3. @elefevre

    Typo

    elefevre authored
  4. @elefevre

    Slightly better comments

    elefevre authored
Commits on Nov 15, 2011
  1. @elefevre
  2. @elefevre
  3. @elefevre

    Removed comments

    elefevre authored
  4. @elefevre

    Import statically

    elefevre authored
  5. @elefevre

    Lots of inlines

    elefevre authored
  6. @elefevre

    Cleaner exception checking

    elefevre authored
  7. @elefevre
  8. @elefevre
  9. @elefevre

    Remove redundant checks

    elefevre authored
  10. @elefevre
  11. @elefevre
  12. @elefevre
  13. @elefevre

    Inlines galore

    elefevre authored
  14. @elefevre
  15. @elefevre

    Spread usage of ASSET_1

    elefevre authored
  16. @elefevre

    Minor refactorings

    elefevre authored
  17. @elefevre

    Better comments

    elefevre authored
  18. @elefevre

    Dead code

    elefevre authored
  19. @elefevre

    All assets are now the same

    elefevre authored
  20. @elefevre
  21. @elefevre

    Factorized paths variables

    elefevre authored
  22. @elefevre
Something went wrong with that request. Please try again.