creste commented on PR #5953: URL: https://github.com/apache/hadoop/pull/5953#issuecomment-1703097859
@anmolanmol1234 - Thank you for the review. I added string constants. Regarding tests, would you mind providing some guidance on what you are expecting? I added [this documentation](https://github.com/apache/hadoop/pull/5953/files#diff-b1b5684dde13128ebedd813dd1b5497dc54d7ce531ddcb105a32043bd4baba22R840-R875) showing how the code changes can be tested using integration tests. I ran the integration tests and copied the output in the merge request description above. If you're expecting unit tests instead of integration tests, then I would appreciate specific guidance on how that can be accomplished. This pull request includes changes to these existing classes: - [AzureADAuthenticator](https://github.com/apache/hadoop/pull/5953/files#diff-dff9c93d1668203c206aa1c092aef9d2921dc6e20af8888d06fae34778991531R135-R155). No existing unit tests exist for this class and I don't see a simple way to mock the HTTP calls. - [AbfsConfiguration](https://github.com/apache/hadoop/pull/5953/files#diff-ce86a30ddecedd6d19c6b61766d31f69454100ba9044d9d80e496e366b841d28R888-R900). No existing unit tests exist to directly test `AbfsConfiguration.getTokenProvider()`. I see that method called a few times indirectly as part of other tests though. This pull request adds a new [WorkloadTokenIdentityProvider](https://github.com/apache/hadoop/pull/5953/files#diff-abfb6ef240071ab286f99e9bd4c5304f6a53fa0e0ac7a3c471c2c0d1a588ed63R35) class that extends `AccessTokenProvider`. Several other classes extend `AccessTokenProvider`, including: - ClientCredsTokenProvider - Does not have unit tests. - CustomTokenProviderAdapter - Has a [single unit test](https://github.com/apache/hadoop/blob/b6d06c89282153a0b5607e6c1236e4138669a1d9/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/extensions/TestCustomOauthTokenProvider.java#L42) with [a comment](https://github.com/apache/hadoop/blob/b6d06c89282153a0b5607e6c1236e4138669a1d9/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/extensions/TestCustomOauthTokenProvider.java#L35-L39) implying E2E integration tests are preferred, but not possible in this case. - MsiTokenProvider - Has an [integration test class](https://github.com/apache/hadoop/blob/b6d06c89282153a0b5607e6c1236e4138669a1d9/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsMsiTokenProvider.java#L47). I'm not sure how this is called when following the [instructions for testing Azure](https://github.com/apache/hadoop/blob/088b5c39b20dc2910995463de764955c737d7898/hadoop-tools/hadoop-azure/src/site/markdown/testing_azure.md). - RefreshTokenBasedTokenProvider - Does not have unit tests. - UserPasswordTokenProvider - Does not have unit tests. Given the lack of unit tests in existing code, is it reasonable to conclude the new integration test documentation and test runs are sufficient in this case? -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
