fix: Use .BrowserApp as default reference browser apk component

[ankushduacodes]

Fixes #2041

[coveralls]

Coverage remained the same at 100.0% when pulling 84702b1 on ankushduacodes:fix_reference_browser_apkComponent_issue into 6030528 on mozilla:master.

[rpl]

Thanks @ankushduacodes this looks to be going in the right direction 👍 .

Besides the small tweaks suggested below, this is also missing additional coverage for the new expected behavior (and that is also pointed out by the coveralls failure).

This can be covered by a new small unit test, a new test case in tests/unit/test-util/test.adb.js, inside the group of tests defined here:

web-ext/tests/unit/test-util/test.adb.js

Line 767 in 6030528

describe('startFirefoxAPK', () => {

Taking a Look to the other tests in that group should help you to figure out how to write the new test (but feel free to ping me and ask me some additional question if you get stuck after giving it a try).

[ankushduacodes]

@rpl I am getting these errors in travis-ci:

⧗   input: Fix reference browser apk component issue
✖   subject may not be empty [subject-empty]
✖   type may not be empty [type-empty]

Could you please explain what are subject and type?

[rpl]

@rpl I am getting these errors in travis-ci:

⧗   input: Fix reference browser apk component issue
✖   subject may not be empty [subject-empty]
✖   type may not be empty [type-empty]

Could you please explain what are subject and type?

@ankushduacodes That's because the pull request title isn't using the commit message conventions that are described here (also linked right below the error):

• https://github.com/mozilla/web-ext/blob/master/CONTRIBUTING.md#writing-commit-messages

It can be easily fixed by just updating the pull request title (e.g. fix: Use .BrowserApp as as the default reference browser apk component would be validate successfully), then the validation should pass when you update the PR again and a new CI job is going to be executed on travis.

---

Follows some more context about the reasons behind the error:

• we do use a convention for the commit message, so that we can generate the changelogs in a semi-automated way
• to prevent landing commits that do not follow the conventions, we do lint them as part of the CI job
• when there is only one commit in the pull request, we only enforce the convention on the commit message
• when there is more then one commit in a pull request, github's "squash and merge" action does use the pull request title as the default message for the generated squashed commit, and so we enforce the convention on the pull request title to avoid merging a commit that doesn't follow the convention into master and missing to notice it.

[rpl]

@ankushduacodes I updated the PR title as described in #2069 (comment) (fix is the conventional-changelog type, everything after the : is the subject, and so the updated PR title does now pass the conventional changelog linting task on the CI).

The only remaining piece for this PR should now be the additional test case to cover the part that coveralls is complaining about, the additional if (...) {} branch added in src/utils/adb.js (the same piece briefly described #2069 (review)).

Let me know how it is going and if you have further questions about that.

[ankushduacodes]

@ankushduacodes I updated the PR title as described in #2069 (comment) (fix is the conventional-changelog type, everything after the : is the subject, and so the updated PR title does now pass the conventional changelog linting task on the CI).

The only remaining piece for this PR should now be the additional test case to cover the part that coveralls is complaining about, the additional if (...) {} branch added in src/utils/adb.js (the same piece briefly described #2069 (review)).

Let me know how it is going and if you have further questions about that.

Hi @rpl, Thank you for fixing the title for me.
I am looking into sinonjs documentation to understand how to tests are being operated. Once I am done I will try to incorporate a test for the if statement as suggested by you. Thank you again for being patient.

[ankushduacodes]

@rpl I have added a unit test to check the proper working of reference browser without specifying an apk component, Please let me what you think

[rpl]

@ankushduacodes thanks! the test you created looks pretty great, good job!

would you mind to also add assertions to double-check that if the firefoxApkComponent parameter is passed as not undefined it isn't going to be overridden with the default one, in the inline review comment that follows I described in more detail how I think we can adapt your test case to be reused for testing both the scenario.

Let me know how that sounds to you.

[rpl]

@ankushduacodes looks great! 🎉 (and I also prefer the function to be nearby the only two test cases that are using it)
The review comment below is just for a very small nitpick (just an inline comment that we don't need anymore), would you mind to remove that comment before I merge this?

Thanks again for contributing this fix!!!

[caitmuenster]

Thanks so much for the patch, @ankushduacodes! 🎉 Your contribution has been added to our recognition wiki.

Welcome onboard! We look forward to seeing you around.

[ankushduacodes]

@caitmuenster Thank you for recognizing my contribution. I just have a small request, I am seeing that on the Link to profile section instead of my github profile link, it links to this pull request, Is it possible to change it?

[caitmuenster]

Absolutely! So sorry about that, @ankushduacodes. It's been fixed now! :)

[ankushduacodes]

Thank you @caitmuenster, I really appreciate that.