Open Bug 1425526 Opened 6 years ago Updated 2 years ago

Add a b-c test to verify description in doorhanger and introduce new helper functions for section testing in utils

Categories

(Toolkit :: Form Autofill, enhancement, P3)

Product:
Toolkit
Component:
Form Autofill
Type:
enhancement

Tracking

()

People

(Reporter: ralin, Unassigned)

References

Details

Attachments

(1 file)

Bug 1425526 - Add a b-c test to verify the description layout of distinct doorhangers.
59 bytes, text/x-review-board-request
steveck
: review+
Details 
We'll need a helper function in b-c test for quickly setup the initial form state, and verify the multiple doorhanger description layout.
Sorry for the late. I have some hard times struggling with the FTU doorhanger. And still have no idea why FTU doorhanger shows up second time within the same serial of b-c tests would make b-c test framework stall. Steve, could you help me review this patch? I think we might will have next commit for refactoring some existing doorhanger tests(or not?). Thanks!
What the Syntax for pushing the Commit to MozReview?
Comment on attachment 8937675 [details]
Bug 1425526 - Add a b-c test to verify the description layout of distinct doorhangers.

https://reviewboard.mozilla.org/r/208390/#review215712

::: browser/extensions/formautofill/test/browser/browser_multiple_section_doorhanger.js:93
(Diff revision 3)
> +  is(addresses.length, 3);
> +});
> +
> +add_task(async function test_submit_sections_updating() {
> +  await BrowserTestUtils.withNewTab({gBrowser, url: MULTIPLE_SECTION_FORM_URL}, async browser => {
> +    await openPopupOn(browser, "#street-address-a");

nit: Maybe we can have another helper function like "selectDropdownItem(browser, selector, index)" or something to wrap up these openPopupOn/synthesizeKey calls.

::: browser/extensions/formautofill/test/browser/head.js:1
(Diff revision 3)
> +/* eslint no-shadow: ["error", { "allow": ["selector", "previouslyFocused", "previouslyIdentified", "profiles"] }] */

Not sure if we need to move this rule out of the original function scope. Any reason?

::: browser/extensions/formautofill/test/browser/head.js:144
(Diff revision 3)
> +  await ContentTask.spawn(browser, {profiles}, async function({profiles}) {
> +    Object.entries(profiles.reduce((acc, profile) => Object.assign(acc, profile), {}))
> +      .map(([id, val]) => [content.document.getElementById(id), val])
> +      .filter(([elem]) => elem)
> +      .forEach(([elem, val]) => {
> +        elem.focus(); // must focus in form to fire a valid submission

nit: To be precise, I remeber the focusin is for identifying autofillFields that could trigger the formfill feature properly. "valid submission" seems not clearly explains why we need focusin here.
Attachment #8937675 - Flags: review?(schung) → review+
(In reply to Harris Jillani from comment #5)
> What the Syntax for pushing the Commit to MozReview?

ah! I'm so sorry, I missed your message. Follow the instruction here: http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html , and then see the further configuration document below page, depend on which version controls client you use. And you'll be able to push your commit simply by `hg push review`(or `git mozreview push` if you use git-cinnabar).
Comment on attachment 8937675 [details]
Bug 1425526 - Add a b-c test to verify the description layout of distinct doorhangers.

https://reviewboard.mozilla.org/r/208390/#review215712

Thanks for reviewing, I'll fix it. And since some helper functions are introduced in this patch, do you think it's a right place to update existing doorhanger tests as well if it can make some line of code concise?

> nit: Maybe we can have another helper function like "selectDropdownItem(browser, selector, index)" or something to wrap up these openPopupOn/synthesizeKey calls.

Good idea, there's a similar one in plain-mochitest which I think we can just crib from.

> Not sure if we need to move this rule out of the original function scope. Any reason?

I found the inline eslint rule apply to entire file instead of function's scope, so maybe it makes more sense to move out to the beginning of file.

> nit: To be precise, I remeber the focusin is for identifying autofillFields that could trigger the formfill feature properly. "valid submission" seems not clearly explains why we need focusin here.

will fix :D
Thanks, the issues are fixed. I'll land the patch if the try is good.

TH result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f38f4fa28cc023b3b88cf7bfb97acb2a8ee23f3a
The test seems not reliable on debug builds, and even the problem is mitigated after adding some delays between autofilling(https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f1109658ba31896a5dde2f7d3037ef0b4be3a2b), still found intermittent failures. We would need either longer delay or doing more promising auto-fill check[0] as we do in m-c. I'll keep tuning the test. Thanks.

[0] https://searchfox.org/mozilla-central/rev/652fbd6270de0d3ec424d2b88f8375ff546f949f/browser/extensions/formautofill/test/mochitest/formautofill_common.js#106-123
Still get the intermittent failure on TH, the last thing I want to do is adding certain period of setTimeout before check, I'll try to find another more promising way.
Assignee: ralin → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: