Github user justinleet commented on the issue:
    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.


Reply via email to