[ https://issues.apache.org/jira/browse/METRON-1421?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16432442#comment-16432442 ]
ASF GitHub Bot commented on METRON-1421: ---------------------------------------- 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. > Create a SolrMetaAlertDao > ------------------------- > > Key: METRON-1421 > URL: https://issues.apache.org/jira/browse/METRON-1421 > Project: Metron > Issue Type: Sub-task > Reporter: Justin Leet > Assignee: Justin Leet > Priority: Major > > Create an implementation of the MetaAlertDao for Solr. This will involve > implementing the various MetaAlertDao methods using the SolrJ library and > also providing a SolrMetaAlertIntegrationTest (similar to > ElasticsearchMetaAlertIntegrationTest). -- This message was sent by Atlassian JIRA (v7.6.3#76005)