Closed Bug 1325538 Opened 5 years ago Closed 4 years ago

Add mochitest for form autofill feature

Categories

(Toolkit :: Form Manager, defect, P3)

Product:
Toolkit
Component:
Form Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: steveck, Assigned: steveck)

References

Details

(Whiteboard: [form autofill:M2])

Attachments

(1 file)

Bug 1325538 - Add mochitest for form autofill feature.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
We'll need a complete mochitest for form autofill once the main autofill architecture is ready.
The patch here is for autocomplete popup test in Bug 1304634, but we can wait for rest of functions(bug 1300989/bug 1300992/bug 1300993) ready and implement a complete mochitest after that.
I'd like to carry on with Steve's patch and make a precedent for mochitest.
Assignee: nobody → ralin
Whiteboard: [form autofill:M1] → [form autofill:M2]
Assignee: ralin → schung
Comment on attachment 8821457 [details]
Bug 1325538 - Add mochitest for form autofill feature.

https://reviewboard.mozilla.org/r/100744/#review132824

::: browser/extensions/formautofill/test/mochitest/test_basic_autocomplete_form.html:8
(Diff revision 2)
> +<head>
> +  <meta charset="utf-8">
> +  <title>Test basic autofill</title>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="text/javascript" src="/tests/SimpleTest/SpawnTask.js"></script>
> +  <script type="text/javascript" src="formautofill_common.js"></script>

I still think that for now we should reference satchel's "satchel_common.js" as-is and then we can have our own .js file for helpers not covered by the satchel one. I would rather we fork only when we need to since it's not exactly nice code to maintain. Are there specific reasons we should fork now?
Attachment #8821457 - Flags: review?(MattN+bmo)
Duplicate of this bug: 1339747
Comment on attachment 8821457 [details]
Bug 1325538 - Add mochitest for form autofill feature.

https://reviewboard.mozilla.org/r/100744/#review135142

Thanks… looking good but a few things to change so far

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:1
(Diff revision 3)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Since this is a test file you don't need to specify a license and it's Public Domain by default.

::: browser/extensions/formautofill/test/mochitest/formautofill_parent_utils.js:49
(Diff revision 3)
> +addMessageListener("add-profile", (msg) => {
> +  ParentUtils.updateProfile("add", "FormAutofill:SaveProfile", msg, "profile-added");
> +});
> +
> +addMessageListener("remove-profile", (msg) => {
> +  ParentUtils.updateProfile("remove", "FormAutofill:Removefile", msg, "profile-removed");

I wonder if these message names should be more unique in case they conflict with other code in the future due to the message manager hierarchies: https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Message_Manager/Message_manager_overview

Maybe prefix with "FormAutofillTest:"?

::: browser/extensions/formautofill/test/mochitest/mochitest.ini:1
(Diff revision 3)
> +[DEFAULT]
> +skip-if = toolkit == 'android'

IIUC You shouldn't need this since we're in the browser/ directory

::: browser/extensions/formautofill/test/mochitest/test_basic_autocomplete_form.html:50
(Diff revision 3)
> +    element.addEventListener("change", function onChange(){
> +      element.removeEventListener("change", onChange);

Nit: use `once`

::: browser/extensions/formautofill/test/mochitest/test_basic_autocomplete_form.html:60
(Diff revision 3)
> +    element.addEventListener("DOMAutoComplete", function onChange(){
> +      element.removeEventListener("DOMAutoComplete", onChange);

Nit: use `once`

::: browser/extensions/formautofill/test/mochitest/test_basic_autocomplete_form.html:104
(Diff revision 3)
> +function* runTests() {
> +  // Form with history only.
> +  yield setupFormHistory();
> +
> +  setupTest("tel", "");
> +  doKey("down");
> +  yield expectPopup();

Can you please use multiple add_task instead? The test that this is based on is hard to follow and everybody who touches it wants to convert it to add_task so I'd rather we don't propagate that previous bad style.

::: testing/profiles/prefs_general.js:366
(Diff revision 3)
>  // Enable form autofill feature testing.
> +user_pref("browser.formautofill.enabled", true);

Hmm… this pref is supposed to default to true for users, right? I'm wondering why have default to false in firefox.js now? It seems like we should change firefox.js to default this to true (like we would ship IIUC) and double-check that we are checking the .experimental pref everywhere necessary.
Attachment #8821457 - Flags: review?(MattN+bmo)
Comment on attachment 8821457 [details]
Bug 1325538 - Add mochitest for form autofill feature.

https://reviewboard.mozilla.org/r/100744/#review135142

> Hmm… this pref is supposed to default to true for users, right? I'm wondering why have default to false in firefox.js now? It seems like we should change firefox.js to default this to true (like we would ship IIUC) and double-check that we are checking the .experimental pref everywhere necessary.

We already had experimental pref checking at bootstrap. Do you think we should postpone the checking to elsewhere?
Comment on attachment 8821457 [details]
Bug 1325538 - Add mochitest for form autofill feature.

https://reviewboard.mozilla.org/r/100744/#review135142

> We already had experimental pref checking at bootstrap. Do you think we should postpone the checking to elsewhere?

No, that seems fine.
Comment on attachment 8821457 [details]
Bug 1325538 - Add mochitest for form autofill feature.

https://reviewboard.mozilla.org/r/100744/#review137084

::: browser/extensions/formautofill/test/mochitest/test_basic_autocomplete_form.html:102
(Diff revision 4)
> +function startTest() {
> +  // Form with history only.
> +  add_task(function* () {

I think add_task can be at the top level and you can remove startTest.

::: browser/extensions/formautofill/test/mochitest/test_basic_autocomplete_form.html:104
(Diff revision 4)
> +  ]);
> +}
> +
> +function startTest() {
> +  // Form with history only.
> +  add_task(function* () {

Please give descriptive names to all of the task functions since they are output in logs and useful for debugging.
Status: NEW → ASSIGNED
Comment on attachment 8821457 [details]
Bug 1325538 - Add mochitest for form autofill feature.

https://reviewboard.mozilla.org/r/100744/#review138670

Thanks
Attachment #8821457 - Flags: review?(MattN+bmo) → review+
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a3f4177a0b2
Add mochitest-plain tests for form autofill. r=MattN
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a49543e6c114
Follow-up to add a missing formAutofillParent.init. r=bustage
(In reply to Pulsebot from comment #18)
> Pushed by mozilla@noorenberghe.ca:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a49543e6c114
> Follow-up to add a missing formAutofillParent.init. r=bustage

Steve, can you check if this change makes sense? Thanks
Flags: needinfo?(schung)
It would not affect the result, so I think that change is fine.
Flags: needinfo?(schung)
https://hg.mozilla.org/mozilla-central/rev/6a3f4177a0b2
https://hg.mozilla.org/mozilla-central/rev/a49543e6c114
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.