Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Add no-op experiment to test service-experiments integration #4551

Merged
merged 4 commits into from
Aug 14, 2019

Conversation

mythmon
Copy link
Contributor

@mythmon mythmon commented Aug 6, 2019

This is for bug 1556751 This change helps verify that the service-experiments library is correctly integrated into Fenix, and that it can correctly enroll users in an experiment. This is exactly the same test that was done in mozilla-mobile/reference-browser#827. That reference browser population was not large enough to completely verify the library's behavior, but no red flags were raised.

I don't think this feature needs tests because it is itself a test of the experiments system.

I'm not sure if this change builds or not. I am having some trouble with the metrics metadata. I'm still working on this.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

@mythmon mythmon marked this pull request as ready for review August 6, 2019 20:52
@mythmon mythmon requested a review from a team as a code owner August 6, 2019 20:52
@mythmon mythmon requested a review from travis79 August 6, 2019 20:52
Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

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

LGTM, pending data review!

@@ -8,7 +8,7 @@
<th>key</th>
<th>type</th>
<th>description</th>
<th>data deview</th>
<th>data review</th>
Copy link
Contributor

Choose a reason for hiding this comment

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

🤦‍♂️ Great catch!

@mythmon
Copy link
Contributor Author

mythmon commented Aug 12, 2019

Data review has passed, and I've updated this with the correct URL for it. I also rebased this PR atop master to fix a merge conflict.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@0c8bcd3). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #4551   +/-   ##
========================================
  Coverage          ?   6.73%           
  Complexity        ?     152           
========================================
  Files             ?     203           
  Lines             ?    8453           
  Branches          ?    1185           
========================================
  Hits              ?     569           
  Misses            ?    7845           
  Partials          ?      39
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/org/mozilla/fenix/FenixApplication.kt 4.16% <0%> (ø) 2 <0> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c8bcd3...f320417. Read the comment docs.

@mythmon
Copy link
Contributor Author

mythmon commented Aug 14, 2019

@travis79 can you take a look at this? I think it is ready to merge.

@travis79
Copy link
Member

Data review has passed, and I've updated this with the correct URL for it. I also rebased this PR atop master to fix a merge conflict.

This looks good to me, my r+ still stands. Because I'm not a codeowner in this repo, we will still need someone with the correct permissions, such as @boek or @colintheshots to review as well.

@travis79
Copy link
Member

I just asked about this in the Fenix-team channel on slack to see if we can get a reviewer for this (or if there is a process we need to follow to get on the radar for reviews)

Copy link
Contributor

@colintheshots colintheshots left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants