Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for mocking final classes in some instrumented tests #1602

Merged
merged 18 commits into from
Jul 26, 2021

Conversation

paulkagiri
Copy link
Contributor

@paulkagiri paulkagiri commented May 4, 2021

This PR is meant to address failures noted in the instrumented tests. The failures were happening because Mockito does not allow mocking final classes and thus a workaround has been arrived where a custom PackageInfo and respective SigningInfo has been developed.

Depends on: AzureAD/microsoft-authentication-library-common-for-android#1454

@paulkagiri paulkagiri added the No-Changelog This request does not update the changelog label May 4, 2021
@iambmelt
Copy link
Member

iambmelt commented May 5, 2021

Grabbing a snapshot of the failing tests, since builds aren't long-term archived

2021-05-04T20:36:35.8361958Z com.microsoft.aad.adal.AuthenticationContextTest > testAcquireTokenByRefreshTokenConnectionNotAvailable[test(AVD) - 11] �[31mFAILED �[0m
2021-05-04T20:36:35.8363361Z 	java.lang.AssertionError: Connection related error code expected:<DEVICE_CONNECTION_IS_NOT_AVAILABLE> but was:<AUTH_REFRESH_FAILED>
2021-05-04T20:36:35.8364432Z 	at org.junit.Assert.fail(Assert.java:88)

2021-05-04T20:37:13.3385788Z com.microsoft.aad.adal.DiscoveryTests > testValidateAuthorityFailedWithoutNetwork[test(AVD) - 11] �[31mFAILED �[0m
2021-05-04T20:37:13.3387226Z 	java.lang.AssertionError
2021-05-04T20:37:13.3387861Z 	at org.junit.Assert.fail(Assert.java:86)

2021-05-04T20:37:13.5367953Z com.microsoft.aad.adal.FileTokenCacheStoreTests > testWriteFileException[test(AVD) - 11] �[31mFAILED �[0m
2021-05-04T20:37:13.5369706Z 	java.lang.NullPointerException: tag is marked non-null but is null
2021-05-04T20:37:13.5370599Z 	at com.microsoft.identity.common.java.logging.Logger.error(Logger.java:171)

2021-05-04T20:37:14.9369258Z com.microsoft.aad.adal.LoggerTest > testCallbackThrowsError[test(AVD) - 11] �[31mFAILED �[0m
2021-05-04T20:37:14.9370322Z 	java.lang.NullPointerException: tag is marked non-null but is null
2021-05-04T20:37:14.9371126Z 	at com.microsoft.identity.common.java.logging.Logger.verbose(Logger.java:332)

2021-05-04T20:37:20.0364080Z com.microsoft.aad.adal.LoggerTest > testCallbackNullMessages[test(AVD) - 11] �[31mFAILED �[0m
2021-05-04T20:37:20.0364984Z 	java.lang.NullPointerException: tag is marked non-null but is null
2021-05-04T20:37:20.0365606Z 	at com.microsoft.identity.common.java.logging.Logger.info(Logger.java:280)

2021-05-04T20:37:23.6374232Z com.microsoft.aad.adal.PackageHelperTests > testGetCurrentSignatureForPackage[test(AVD) - 11] �[31mFAILED �[0m
2021-05-04T20:37:23.6375269Z 	java.lang.AssertionError: should be same info expected:<lL7Wg+FHYAV2pyuhm5vyJayWJqE=> but was:<null>
2021-05-04T20:37:23.6376048Z 	at org.junit.Assert.fail(Assert.java:88)

2021-05-04T20:37:29.7392776Z > Task :adal:connectedLocalDebugAndroidTest FAILED
2021-05-04T20:37:29.7393445Z :adal:connectedLocalDebugAndroidTest (Thread[Daemon worker,5,main]) completed. Took 2 mins 46.892 secs.
2021-05-04T20:37:29.7395112Z 
2021-05-04T20:37:29.7395684Z Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
2021-05-04T20:37:29.7397055Z Use '--warning-mode all' to show the individual deprecation warnings.
2021-05-04T20:37:29.7398017Z See https://docs.gradle.org/6.7.1/userguide/command_line_interface.html#sec:command_line_warnings
2021-05-04T20:37:29.7398694Z 58 actionable tasks: 58 executed
2021-05-04T20:37:29.7399059Z 
2021-05-04T20:37:29.7399450Z FAILURE: Build failed with an exception.
2021-05-04T20:37:29.7399773Z 
2021-05-04T20:37:29.7400105Z * What went wrong:
2021-05-04T20:37:29.7401068Z Execution failed for task ':adal:connectedLocalDebugAndroidTest'.
2021-05-04T20:37:29.7401902Z > There were failing tests. See the report at: file:///home/gradle/adal/build/reports/androidTests/connected/flavors/localDebugAndroidTest/index.html
2021-05-04T20:37:29.7402530Z 
2021-05-04T20:37:29.7402861Z * Try:
2021-05-04T20:37:29.7403913Z Run with --stacktrace option to get the stack trace. Run with --debug option to get more log output. Run with --scan to get full insights.
2021-05-04T20:37:29.7404588Z 
2021-05-04T20:37:29.7405204Z * Get more help at https://help.gradle.org
2021-05-04T20:37:29.7405651Z 
2021-05-04T20:37:29.7406023Z BUILD FAILED in 7m 33s
2021-05-04T20:39:29.2219200Z ##[error]Bash exited with code '1'.
2021-05-04T20:39:29.2252178Z ##[section]Finishing: Build and test inside docker container
``

Copy link
Member

@iambmelt iambmelt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

License files

@iambmelt
Copy link
Member

iambmelt commented May 5, 2021

More or less, these changes seem fine - the tests broken in #1587 seem to be resolved. Let's fix the license declarations in these files, resolve the submodule conflict and send it round for another signoff

@paulkagiri paulkagiri requested a review from a team as a code owner May 5, 2021 04:55
@paulkagiri
Copy link
Contributor Author

testGetCurrentSignatureForPackage is also failing because of #1587 but after spending countless hours trying to fix it, am suspending my investigations for now to pick up the other pending work.

@shahzaibj
Copy link
Contributor

I think we should update the description here because as I understand there were really a couple of issues:

  • Mocking of final classes
  • Accessing inaccessible classes located in androidTest of common and trying to use those in tests in ADAL

I think this PR addresses both of those issues and we should call that out.

@paulkagiri paulkagiri changed the title Instrumented tests fix Fix for mocking final classes in some instrumented tests May 26, 2021
Copy link
Member

@rpdome rpdome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a minor feedback

Copy link
Contributor

@shahzaibj shahzaibj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can make the pipeline pass...then I can approve it

@rpdome
Copy link
Member

rpdome commented Jun 8, 2021

@paulkagiri this is blocking my PR, could we get this fixed and merge? Thanks!

rpdome
rpdome previously requested changes Jun 8, 2021
Copy link
Member

@rpdome rpdome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulkagiri . I disabled the tests for now, but we should definitely get this up and running by end of month.

@paulkagiri paulkagiri force-pushed the paul/fix-instrumented-tests branch from 9e6099e to f776087 Compare June 9, 2021 09:00
Copy link
Contributor

@AdamBJohnsonx AdamBJohnsonx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comment

@paulkagiri paulkagiri dismissed stale reviews from iambmelt and rpdome July 26, 2021 08:47

Already addressed the requested changes

@paulkagiri paulkagiri merged commit 390c889 into dev Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No-Changelog This request does not update the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants