Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

For #1481. Use androidx runner in few more modules. #3415

Merged
merged 4 commits into from
Jun 13, 2019
Merged

For #1481. Use androidx runner in few more modules. #3415

merged 4 commits into from
Jun 13, 2019

Conversation

dector
Copy link
Contributor

@dector dector commented Jun 12, 2019 •
edited
Loading

Issue #1481

  • Enable includeAndroidResources and enableUnitTestBinaryResources for feature-sitepermissions module.
  • Enable includeAndroidResources and enableUnitTestBinaryResources for feature-readerview module.
  • Enable includeAndroidResources and enableUnitTestBinaryResources for service-experiments module.
  • Turn on enableUnitTestBinaryResources globally (exclude few modules explicitly).
  • Use AndroidJUnit4 as a test runner (from AndroidX Test Ext).

Complexity

Easy (★☆☆)

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

@dector dector requested a review from a team as a code owner June 12, 2019 16:52
@dector dector changed the title For #1481. Use androidx runner in feature-sitepermissions. For #1481. Use androidx runner in few more modules. Jun 12, 2019
@Amejia481 Amejia481 added the 🛬 needs landing PRs that are ready to land label Jun 12, 2019
@dector
Copy link
Contributor Author

dector commented Jun 12, 2019

@Amejia481 I have one more commit for service-experiments. Should I create new PR or push it here?

@Amejia481 Amejia481 removed the 🛬 needs landing PRs that are ready to land label Jun 12, 2019
@Amejia481 Amejia481 self-requested a review June 12, 2019 18:30
@Amejia481
Copy link
Contributor

@dector push it here!

@dector dector force-pushed the fix-1481-feature-sitepermissions branch from 7244467 to 272c5ed Compare June 12, 2019 18:35
@dector dector requested a review from a team as a code owner June 12, 2019 18:35
@dector
Copy link
Contributor Author

dector commented Jun 12, 2019

Oh, I've missed to unignore one test. One moment please. 🤦‍♂️

@Amejia481
Copy link
Contributor

No worries, just let me know when you finish :)

@dector dector force-pushed the fix-1481-feature-sitepermissions branch from 272c5ed to 8f57843 Compare June 12, 2019 19:22
@dector
Copy link
Contributor Author

dector commented Jun 12, 2019 •
edited
Loading

@Amejia481 Done. It took bit more than a while. :) Had some issues with new activity scenario in ExperimentsDebugActivityTest but now they are resolved.

Amejia481
Amejia481 approved these changes Jun 12, 2019
View reviewed changes
Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

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

LGTM!

travis79
travis79 approved these changes Jun 12, 2019
View reviewed changes
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.

This is great!

I have been working on a PR in the experiments component and this will make for a challenging rebase, so I may have some questions for you around testing bits after this lands and I rebase on it.

...ice/experiments/src/test/java/mozilla/components/service/experiments/ActiveExperimentTest.kt Outdated
}

@Test
fun `test load invalid experminet from storage`() {
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
fun `test load invalid experminet from storage`() {
fun `test load invalid experiment from storage`() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. I think uncle Sigmund can be proud of me. 😅

Thanks!

@dector
Copy link
Contributor Author

dector commented Jun 12, 2019 •
edited
Loading

@travis79 @Amejia481 Let's remove service-experiments from this PR? I suggest it should be merged after new implementation because applying this changes are much easier than rebasing new tests/fixes on 'em.

@travis79
Copy link
Member

@travis79 @Amejia481 Let's remove service-experiments from this PR? I suggest it should be merged after new implementation because applying this changes are much easier than rebasing new tests/fixes on 'em.

Thanks! I expect the bulk of the changes I'm working on to be done in the next two weeks, I just know that I've touched the ExperimentsTest.kt quite a bit and it might be tricky.

@dector
Copy link
Contributor Author

dector commented Jun 12, 2019

Probably it'll be good idea to include only minimal necessary changes without minor readability improvements.

@dector dector force-pushed the fix-1481-feature-sitepermissions branch from 8f57843 to cad8bd1 Compare June 12, 2019 23:20
@dector
Copy link
Contributor Author

dector commented Jun 12, 2019

Done. With current changes rebasing on this revision should reduce conflicts in service-experiments almost to zero (some conflicts in imports are possible).

@travis79
Copy link
Member

Thanks @dector, I hope to have the rest of the changes I'm working on done soon so I appreciate sparing me from the merge conflict. 😄

@dector
Copy link
Contributor Author

dector commented Jun 13, 2019

@Amejia481 Pls pay attention to latest commit. I've turned on enableUnitTestBinaryResources globally. :)

@dector dector force-pushed the fix-1481-feature-sitepermissions branch from 6fa4e84 to 6c8227c Compare June 13, 2019 00:57
@dector dector force-pushed the fix-1481-feature-sitepermissions branch from 6c8227c to ba8510c Compare June 13, 2019 01:15
pocmo
pocmo approved these changes Jun 13, 2019
View reviewed changes
Copy link
Contributor

@pocmo pocmo left a comment

Choose a reason for hiding this comment

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

👍 Great work, @dector!

@pocmo pocmo merged commit 3e1a428 into mozilla-mobile:master Jun 13, 2019
@dector dector deleted the fix-1481-feature-sitepermissions branch June 13, 2019 12:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pocmo pocmo pocmo approved these changes

@Amejia481 Amejia481 Amejia481 approved these changes

@travis79 travis79 travis79 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dector @Amejia481 @travis79 @pocmo