vinayakphegde commented on PR #6717: URL: https://github.com/apache/hbase/pull/6717#issuecomment-2917299012
> > > 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? -- 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]
