Skip to content

More cleanup in ArchiveTestBase #46

Open
wants to merge 26 commits into from

2 participants

@elefevre

So, this is what I was thinking of.
The ArchiveTestBase file is now 625 lines long (down from ~1300).

I'm going to stop refactoring now and wait for your comments.

@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
ShrinkWrap member

Super.

@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
ShrinkWrap member

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
ShrinkWrap member

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
ShrinkWrap member

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
ShrinkWrap member

+1, good centralization and DRY.

@ALRubinger
ShrinkWrap member

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
ShrinkWrap member

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
ShrinkWrap member

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.

ShrinkWrap member
@ALRubinger
ShrinkWrap member

+1 on all the ASSET_1 stuff

@ALRubinger
ShrinkWrap member

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
Something went wrong with that request. Please try again.