search

iipc / webarchive-commons

Common web archive utility code.
Apache License 2.0
50 stars 72 forks source link

fix for https://github.com/iipc/webarchive-commons/issues/38 - detect end of http protocol headers in a smarter way, to avoid calling write(byte) repeatedly; add unit tests #39

Closed nlevitt closed 9 years ago

kris-sigur commented 9 years ago

Why isn't TmpDirTestCase.java in the "test area"?

nlevitt commented 9 years ago

Why isn't TmpDirTestCase.java in the "test area"?

Because heritrix unit tests use it too. It has been in heritrix-commons, also not in the "test area", because heritrix-modules, heritrix-engine, etc tests also use it.

nlevitt commented 9 years ago

Hey folks, not to be annoying, but we are eager to get this merged, so we can avoid needing to fork...

anjackson commented 9 years ago

Apologies for the delay. Can you add a note describing the change to CHANGES.md under 1.1.5 and we'll get on an roll a release.

kris-sigur commented 9 years ago

I'm still a bit off put by having test code under main. I recognize that that is the way it is in Heritrix now, but it feels like that is something that should be fixed rather than perpetuated.

Looking at Heritrix, it seems that the only references to to TmpDirTestCase are from other classes that can be moved to the test side of things (TestUtils and ModuleTestBase) and those classes are again referenced only from classes that should be moved as well. All told its only 6 files (https://github.com/kris-sigur/heritrix3/compare/testfix).

kris-sigur commented 9 years ago

And I now realize why this doesn't work and withdraw my objections. I hadn't realized how exactly Maven handles this.

kris-sigur commented 9 years ago

Although, the model described here would be cleaner: http://stackoverflow.com/a/14724000/73652

anjackson commented 9 years ago

Okay, moving to a cleaner model looks fiddly to do right now. So, I suggest that, once we have an updated to CHANGES.md, we accept this pull request and roll a release, but with the test-jar goal added to the POM (as in @kris-sigur's link). Then 1.1.5 will have a test-jar artefact in the release. For the following release, we can move the test utility class over to the test area, and H3 and add that as a dependency. Sound reasonable?

nlevitt commented 9 years ago

Thanks for the quick response. I added an entry to CHANGES.md. I agree switching to this other model for test dependencies is a separate issue from this one, but the plan sounds fine to me.

kris-sigur commented 9 years ago

@anjackson Sounds like a plan to me. Merging.

nlevitt commented 9 years ago

Thanks guys