anmolnar commented on PR #6717: URL: https://github.com/apache/hbase/pull/6717#issuecomment-2919423263
> > > > Overall lgtm, but surprising how low the level of unit tests' granularity is: for instance `BackupAdminImpl` is such a big class and there's no corresponding test class. It has a lot of private methods which could be tested as "units" if they were package private and we had a `BackupAdminTest` class. Please consider that. > > > > > > > > > True, there isn’t a separate test class specifically for `BackupAdminImpl`. However, if you look at the overall structure, the core implementation resides in `BackupAdminImpl`, while the other classes primarily handle input preparation (including validation) and delegate the calls to `BackupAdminImpl`. We have a lot of tests for those classes, which indirectly cover almost all parts of `BackupAdminImpl`. > > > That said, I’ll still take a closer look at the class and add tests if I find any gaps. > > > > > > Could you please provide examples of that? Are they really just proxy calls? > > Yes, for example, mergeBackups() is already covered by tests in TestBackupMerge, TestIncrementalBackupMergeWithBulkLoad, and IntegrationTestBackupRestore. The checkIfValidForMerge() method is private and is only called from mergeBackups(), which is the case for most of the methods in this class. > > That said, we can definitely create a dedicated test class for this and add the remaining/uncovered test cases. I’ll open a separate Jira for that. Does that sound good? Yeah, these examples are all integration tests as the same suggests in some cases: they create "real"/mini HBase cluster, sets up a connection, get reference to Admin and call methods which are publicly accessible. Looks like this is historically the main approach for HBase testing, but this is not unit testing. What I'm talking about is instantiating BackupAdminImpl class directly with a mocked connection and test the methods individually. In order to do this you have to make methods accessible from unit tests, for instance, by changing visibility to package private. I don't insist doing this strictly, just leaving here as something to think about. Let me approve this patch regardless. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
