Closed Bug 1393374 Opened 2 years ago Closed 2 years ago

[Form Autofill] Add test cases to check the search result that fallback from credit card fields

Categories

(Toolkit :: Form Manager, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: ralin, Assigned: ralin)

References

Details

(Whiteboard: [form autofill:MVP])

Attachments

(3 files, 1 obsolete file)

Right now, we've made pref account for one of the fallback conditions, but still lack a test case for credit card field in test_basic_autocomplete_form.html.
Not only for pref, we will need all equivalents of what we've done for address as well.
Summary: [Form Autofill] Add a test case to check if startSearch fallback from credit card field when credit card is pref off → [Form Autofill] Add test cases to check the search result that fallback from credit card fields
Assignee: nobody → ralin
Comment on attachment 8904294 [details]
Bug 1393374 - Part 2. Add a credit card basic autofill mochitest.

https://reviewboard.mozilla.org/r/176068/#review182692

Looks good.

::: browser/extensions/formautofill/test/mochitest/test_basic_creditcard_autocomplete_form.html:71
(Diff revision 1)
> +
> +function checkFormFilled(creditCard) {
> +  let promises = [];
> +  for (let prop in creditCard) {
> +    let element = document.getElementById(prop);
> +    let converted = "" + creditCard[prop]; // Convert potential number to string

nit: `String(creditCard[prop])`

::: browser/extensions/formautofill/test/mochitest/test_basic_creditcard_autocomplete_form.html:199
(Diff revision 1)
> +  // Explicitly focus on the other field to receive "change" event from the original focused input.
> +  await setInput("#cc-csc", "");

That makes sense but I wonder why we didn't do this in the address' version?
Attachment #8904294 - Flags: review?(lchang) → review+
Comment on attachment 8903626 [details]
Bug 1393374 - Part 1. Introduce credit card helper functions in formautofill mochitest.

https://reviewboard.mozilla.org/r/175398/#review183682

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:53
(Diff revision 3)
>  }
>  
>  function checkMenuEntries(expectedValues, isFormAutofillResult = true) {
>    let actualValues = getMenuEntries();
>    // Expect one more item would appear at the bottom as the footer if the result is from form autofill.
> -  let expectedLength = isFormAutofillResult ? expectedValues.length + 1 : expectedValues.length;
> +  let expectedLength = expectedValues.length + (+!!isFormAutofillResult);

nit: I feel the original one looks clearer.

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:61
(Diff revision 3)
>    for (let i = 0; i < expectedValues.length; i++) {
>      is(actualValues[i], expectedValues[i], " Checking menu entry #" + i);
>    }
>  }
>  
> -async function addAddress(address) {
> +function invokeAsyncChromeTask(msg, res, payload = {}) {

Not sure what `res` stands for. (response?) I'd prefer to use full names (including "message" for `msg`).

::: browser/extensions/formautofill/test/mochitest/formautofill_parent_utils.js:54
(Diff revision 3)
> -      if (data != type) {
> -        return;
> +    switch (collection | OPS[type]) {
> +      case (COLLECTIONS.address | OPS.add): {

I kind of feel it's overkill since those combinations won't appear elsewhere and won't be reused. Maybe we can discuss it face to face.
Attachment #8903626 - Flags: review?(lchang)
Comment on attachment 8903626 [details]
Bug 1393374 - Part 1. Introduce credit card helper functions in formautofill mochitest.

https://reviewboard.mozilla.org/r/175398/#review183682

> nit: I feel the original one looks clearer.

reverted to the original one.

> Not sure what `res` stands for. (response?) I'd prefer to use full names (including "message" for `msg`).

fixed

> I kind of feel it's overkill since those combinations won't appear elsewhere and won't be reused. Maybe we can discuss it face to face.

Yeah, agree. Seems no other place will re-use it and also no new operator or collection will be added in the near future. Thanks.
Comment on attachment 8903626 [details]
Bug 1393374 - Part 1. Introduce credit card helper functions in formautofill mochitest.

https://reviewboard.mozilla.org/r/175398/#review185392

Looks good. Thanks.
Attachment #8903626 - Flags: review?(lchang) → review+
Got the same error on other mochitests with local debug build, so I suspect that the problem is something rather than this patch. Will try to re-build on top of earlier commit and test again.
I've tried different reduced cases, but the part2 patch still cause a permafail with nsTArray_base leaked. Since Sean need those utils to write credit card mochitests, I'm thinking to split the part 1 off to another bug and land it first. Let's discuss in person next week.
Even a test of the simple form is with one input only, `elem.focus()` still makes the memory leak.

with focus:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1f24ba39a993052cc9828849d587fbe4d914082

without focus:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bbc850f799f6e4d17819c3dbfa4b4c69a715b67

These tests are without `addEventListener("focusin", this);` in FormAutofillFrameScript, so I guess this issue is related to `focus()` or other `focusin` handlers.
Does anyone have any idea that which focus related code causes this leak?
I'd like to give it a shot on autoland since it seems unlikely to be our problem from Sean's investigation.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5033a269b87f
Part 1. Introduce credit card helper functions family in formautofill mochitest. r=lchang
https://hg.mozilla.org/integration/autoland/rev/ed09c905bce8
Part 2. Add a credit card basic autofill mochitest. r=lchang
Keywords: checkin-needed
Hi Eric, nice to meet you over bugzilla :D

I'm sorry to bother you, but we've encountered an memory leak issue and been having hard time to find the cause. We don't have too many clues of potential reasons, the closest one after times of narrow down is on comment 16 that Sean made a reduced test case that even only .focus() on an input element would introduce 8 bytes leaks on default process. Could you help us to point out the possible cause if you got some times? also not sure those are informative enough, we're willing to provide more context if needed. Thank you very much!
Flags: needinfo?(ralin) → needinfo?(erahm)
See Also: → 1401454
I've filed bug 1401454 to follow-up the memory leak, and I think this should not be the blocker on the way for other more important patches like  1398101, perhaps, we should go with skip-if=debug first until figuring out the cause of failure. Thanks.
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0735f44b742a
Part 1. Introduce credit card helper functions in formautofill mochitest. r=lchang
https://hg.mozilla.org/integration/autoland/rev/a182935e2bd4
Part 2. Add a credit card basic autofill mochitest. r=lchang
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0735f44b742a
https://hg.mozilla.org/mozilla-central/rev/a182935e2bd4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Thank you Luke, Sebastian :D
This is permafailing on Beta. Please fix the test or backout immediately.
Flags: needinfo?(ralin)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #31)
> This is permafailing on Beta. Please fix the test or backout immediately.

checking, thanks
Presumably the test assumes extensions.formautofill.creditCards.enabled will be set and that isn't currently the case for Beta?
Thanks for the information, I'll attach a patch to explicitly pref on: http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/browser/app/profile/firefox.js#1706-1710
Flags: needinfo?(ralin)
I meant in the test :D
I ended up adding a condition in mochitest.ini to stop the test from running on other releases.
Attachment #8910279 - Flags: review?(lchang)
Attachment #8910279 - Flags: review?(MattN+bmo)
Per IRC discussion with Luke, we're just going to backout for now.
https://hg.mozilla.org/mozilla-central/rev/469eb992a9d166004f2601ce725786f671219054
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla57 → ---
BTW, you may find the XPCOM_MEM_LEAK_LOG env var useful for tracking down that debug leak.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #39)
> Per IRC discussion with Luke, we're just going to backout for now.
No problem, far enough we should not hurriedly get into a wallpaper fix for this. I'll have more discussion with Luke.

> BTW, you may find the XPCOM_MEM_LEAK_LOG env var useful for tracking down
> that debug leak.
Will try, thank you for your help :-)
Andrew do you think you help :ralin out (see comment 23)?
Flags: needinfo?(erahm) → needinfo?(continuation)
If you set the XPCOM_MEM_LOG_CLASSES variable to nsStringBuffer it will show you the allocation stack for the string buffer. That might help you figure out what is going wrong. There's some information about how to set that when running locally and on TreeHerder here: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/BloatView#Bloat_Statistics_on_Tinderbox

Let me know if you need further help.
Flags: needinfo?(continuation)
Comment on attachment 8910279 [details] [diff] [review]
Bug-1393374-Only-run-form-autofill-credit-card-mochitest.patch

As patches have backout'ed, let's take our time to fix it properly in the original patch.
Attachment #8910279 - Flags: review?(lchang)
Attachment #8910279 - Flags: review?(MattN+bmo)
It looks like now it's able to set pref before running mochitests, though still no precedents did that. Credit card related tests now can run w/o caring about which release it's in, we'll have to keep the setting until no features hides behind prefs. Will flag checkin-needed once Luke agreed with this approach. Thanks.
(In reply to Ray Lin[:ralin] from comment #47)
> It looks like now it's able to set pref before running mochitests, though still no precedents did that.

We did the same thing in our browser chrome test already.

> Will flag checkin-needed once Luke agreed with this approach. Thanks.

I'm fine with that since the credit-card code is quite coupled with address' code. It seems a bit hard to disable credit-card tests one by one manually.

Thanks for investigating this.
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c7fd224bfe0
Part 1. Introduce credit card helper functions in formautofill mochitest. r=lchang
https://hg.mozilla.org/integration/autoland/rev/61af19cbe6bf
Part 2. Add a credit card basic autofill mochitest. r=lchang
Comment on attachment 8910279 [details] [diff] [review]
Bug-1393374-Only-run-form-autofill-credit-card-mochitest.patch

no longer need it.
Attachment #8910279 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/2c7fd224bfe0
https://hg.mozilla.org/mozilla-central/rev/61af19cbe6bf
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1404811
You need to log in before you can comment on or make changes to this bug.