Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@nickwallen
Assuming we go through with the partial refactoring, the existence of those
interfaces does not prevent us from testing the individual DAOs. A lot of the
highly specific changes at the implementation level of MetaAlertDao are things
that more or less need to be integration tested (e.g. making sure the magic
Solr incantations do what's expected). The other stuff can be unit tested
regardless of what implementation class it's in.
Re: #832 Absolutely agree we should have as many unit tests as possible to
avoid things like this.
The tests are substantially better than they were prior to this PR. I'm
happy to take another pass to try to catch out any cases we're missing. The
initial pass took place during a refactoring and I wouldn't be surprised if
there's more we can do. The problem is that I need to have a stable code base
that I'm testing against. That's the main reason I'm trying to get this
narrowed down and resolve the overall code structure. I'm not trying to say
that we're done adding tests or that there's not room for improvement. I'm
trying to get to a point where I'm not trying to build the tests we'd both like
to see on a pile of sand. For me, killing that dedupe is a prerequisite
because it could change how those unit tests happen in terms of avoiding
duplication and ensuring that the variants on the implementations are more
easily understood and tested. Right now it's hard to build expectations
against the code, because we're changing basic structure and how things are
built. Even if
the test cases themselves cover the same thing, a lot of stuff like setup
changes so there's nontrivial effort involved in keeping the tests aligned.
IMO (and feel free, or even encouraged, to argue with me on this), killing
the interfaces doesn't buy us enough testability to merit adding that
refactoring to this PR. I do think rethinking the DAO structure is a
definitely a worthwhile task that could improve a lot of things at once, but it
should be a separate effort.
I think a reasonable level of testing for this PR is the integration
testing at the level of broad interface DAOs + the already substantial increase
in testing + maybe another pass once the implementation has stabilized. Let me
know if your expectations are different, because it sounds like both of our
expectations are influencing how much refactoring we want, so we need to be on
the same page there.
Re: the issue on this PR itself, that appears to be an issue with using the
correct source.type field for Solr. I'm working on a confirming and providing a
separate fix for that. Part of the problem appears to be the unit tests
themselves are incorrect. The other problem is that our test schemas
incorrectly use ':' instead of '.' and I carried this through. So the problem
isn't that the tests are incomplete, but that it's compounding a bug in our
tests themselves. In fact, I called out that I thought it was weird we were
using ':' in the PR description.
---
