Skip to content

Fix personalization test and add check for optionalProvider. #2562

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

Merged
merged 1 commit into from
Apr 1, 2021

Conversation

rlazo
Copy link
Collaborator

@rlazo rlazo commented Apr 1, 2021

The personalization test was using the default namespace, and because another instance of config already existed in that namespace, the test instance was not being created. This caused the personalization test to try to use real objects instead of the mocks, including the direct executor. This PR fixes this and uses a provider to fake the interaction when Analytics is present and when it's not.

@rlazo rlazo requested review from vkryachko and danasilver April 1, 2021 05:13
@google-cla google-cla bot added the cla: yes Override cla label Apr 1, 2021
@google-oss-bot
Copy link
Contributor

Coverage Report

Affected SDKs

  • firebase-config

    SDK overall coverage changed from 88.53% (0a5b6ba) to 88.82% (1e52d552) by +0.29%.

    Filename Base (0a5b6ba) Head (1e52d552) Diff
    ConfigGetParameterHandler.java 95.04% 96.45% +1.42%
    Personalization.java 88.57% 91.43% +2.86%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (1e52d552) is created by Prow via merging commits: 0a5b6ba c82ba25.

@google-oss-bot
Copy link
Contributor

Binary Size Report

Affected SDKs

No changes between base commit (0a5b6ba) and head commit (1e52d552).

Test Logs

Notes

Head commit (1e52d552) is created by Prow via merging commits: 0a5b6ba c82ba25.

Copy link
Member

@vkryachko vkryachko left a comment

Choose a reason for hiding this comment

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

I wonder if the perf test failure a flake or is caused by the change in RC

@rlazo
Copy link
Collaborator Author

rlazo commented Apr 1, 2021

/test check-changed

1 similar comment
@rlazo
Copy link
Collaborator Author

rlazo commented Apr 1, 2021

/test check-changed

@rlazo
Copy link
Collaborator Author

rlazo commented Apr 1, 2021

I wonder if the perf test failure a flake or is caused by the change in RC

Flake

Copy link
Member

@vkryachko vkryachko left a comment

Choose a reason for hiding this comment

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

LGTM, but let's wait for @danasilver to review before merging.

@rlazo
Copy link
Collaborator Author

rlazo commented Apr 1, 2021

SGTM

@rlazo rlazo merged commit 98f7de8 into master Apr 1, 2021
@rlazo rlazo deleted the rl.tcv_rc branch April 1, 2021 20:14
@firebase firebase locked and limited conversation to collaborators May 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Override cla size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants