Closed Bug 1349489 Opened 8 years ago Closed 8 years ago

Implement the test fixtures for the top 12 web sites.

Categories

(Toolkit :: Form Manager, enhancement, P1)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

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

People

(Reporter: selee, Assigned: selee)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M2])

Attachments

(2 files)

Bug 1349489 - Part 1: Move the codes from FormAutofillHandler.collectFormFields to FormAutofillHeuristics.getFormInfo.;
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1349489 - Part 2: Add test fixtures for autofill field name heuristics.;
59 bytes, text/x-review-board-request
MattN
: review+
Details
Since PRD defines the following sites as the top priority target, we should implement the test fixtures for these sites to verify the prediction in xpc-shell test. Amazon Walmart Staples Sears Macy’s Office Depot CDW Corp. Home Depot Best Buy Liberty Interactive (QVC.com) Newegg Costco
Whiteboard: [form autofill:M2]
Hey MattN, I summarize some points for this WIP patch: 1. Include the test fixtures of top 12 web sites. 2. Implement createTestDocumentFromFile in MockDocument for loading and parsing HTML fixtures. 3. Add autocomplete_basic.html to verify the autocomplete attribute. Macy's includes autocomplete attribute as well, and it can be verified in the test now. Could you take a look the current patch? I am going to add TODO for every test fixture.
Blocks: 1347176
Some additional comments explain the details: * "fixtures" folder is placed at the same level with "unit" and "browser" folder because the fixtures can be shared in both xpcshell test and mochitest. * createTestDocumentFromFile is for reading a html and parse it into HTMLDocument. Because "parseFromStream" can not convert it into HTML one, that's why the file is read as string. * For the test fixtures, the unnecessary attribute and elements are removed from all test fixtures. I am trying to keep the fixtures as simple as possible. * "title" and "autofill-prediction" attribute will be removed eventually in this patch. It's a useful reference when I implement the prediction in expected result.
Comment on attachment 8851202 [details] Bug 1349489 - Part 1: Move the codes from FormAutofillHandler.collectFormFields to FormAutofillHeuristics.getFormInfo.; https://reviewboard.mozilla.org/r/123570/#review129180 I think the main thing is to check with Gerv if we need to do anything from a licensing perspective. ::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:54 (Diff revision 4) > + element, // TODO: Apply Cu.getWeakReference and use get API for strong ref. > + }; FYI: This will need rebasing ::: browser/extensions/formautofill/test/fixtures/Walmart/Shipping.html:46 (Diff revision 4) > + <div> > + <div> > + <div> > + <div>*required field</div> > + <label><span><span><span>First name*</span></span></span></label> > + <div><input title="First name" label="[object Object]" name="firstName" type="text"></div> Did they really have "[object Object]" as their label many times? ::: browser/extensions/formautofill/test/unit/test_fixtures.js:3 (Diff revision 4) > +XPCOMUtils.defineLazyModuleGetter(this, "FormLikeFactory", > + "resource://gre/modules/FormLikeFactory.jsm"); Nit: No need use lazy loading in tests so just use Cu.import like the others ::: browser/extensions/formautofill/test/unit/test_fixtures.js:116 (Diff revision 4) > + {"section":"", "addressType":"", "contactType":"", "fieldName":"address-level2", "element": {}}, > + {"section":"", "addressType":"", "contactType":"", "fieldName":"postal-code", "element": {}}, > + {"section":"", "addressType":"", "contactType":"", "fieldName":"tel", "element": {}}, It would be nice to not have to duplicate `"element": {}` every time unless you plan on using it? ::: browser/extensions/formautofill/test/unit/test_fixtures.js:249 (Diff revision 4) > + do_print("Starting test pattern: " + testPattern.description); > + let file = do_get_file(FIXTURE_PATH_PREFIX + testPattern.fixturePath); Do you plan to have multiple tests for a single file? If not, we probably don't need mandatory descriptions and can use testPattern.fixturePath in the `do_print`. ::: browser/extensions/formautofill/test/unit/test_fixtures.js:255 (Diff revision 4) > + for (let field of doc.getElementsByTagName("input")) { > + let formLike = FormLikeFactory.createFromField(field); > + if (!forms.some(form => form.rootElement === formLike.rootElement)) { > + forms.push(formLike); > + } > + } We should eventually have a (test?) helper for this since it's a common pattern and going to need to change when we support textarea and select (in which case it should use `querySelectorAll`. ::: toolkit/modules/tests/modules/MockDocument.jsm:54 (Diff revision 4) > + let converter = Cc["@mozilla.org/intl/converter-input-stream;1"]. > + createInstance(Ci.nsIConverterInputStream); > + converter.init(fileStream, "UTF-8", 0, 0); > + > + let str = {}; > + let data = ''; > + let read = 0; > + do { > + read = converter.readString(0xffffffff, str); > + data += str.value; > + } while (read != 0); > + > + converter.close(); You should use `NetUtil.readInputStreamToString(fileStream, fileStream.available());`
Attachment #8851202 - Flags: review?(MattN+bmo)
Gerv, we would like to use derivatives of real websites as test cases for Form Autofill which I would hope is fine under fair use rules. Do we need to do anything special to clarify the license e.g. that it doesn't fall under the default PD license for tests?
Assignee: nobody → selee
Status: NEW → ASSIGNED
Flags: needinfo?(gerv)
Comment on attachment 8851202 [details] Bug 1349489 - Part 1: Move the codes from FormAutofillHandler.collectFormFields to FormAutofillHeuristics.getFormInfo.; https://reviewboard.mozilla.org/r/123570/#review129180 > Did they really have "[object Object]" as their label many times? label="[object Object]" exists in their web pages, and it looks like a mistake. Besides, it's useless for our heuristics, so I remove them. > It would be nice to not have to duplicate `"element": {}` every time unless you plan on using it? It's only for Assert.deepEqual. I remove them and assign elementWeakRef to each field for Assert.deepEqual. > Do you plan to have multiple tests for a single file? If not, we probably don't need mandatory descriptions and can use testPattern.fixturePath in the `do_print`. No, one path is used once in all patterns. I remove the description and use testPattern.fixturePath as description.
Comment on attachment 8851202 [details] Bug 1349489 - Part 1: Move the codes from FormAutofillHandler.collectFormFields to FormAutofillHeuristics.getFormInfo.; https://reviewboard.mozilla.org/r/123570/#review129180 > We should eventually have a (test?) helper for this since it's a common pattern and going to need to change when we support textarea and select (in which case it should use `querySelectorAll`. `querySelectorAll` is in the latest patch. `select` and `textarea` elements are refused to be applied `FormLikeFactory.createFromField` with this error message [1]: "Error: createFromField requires a field in a document" I suppose this will be fixed when implementing the support of `select` and `textarea` tags. [1] http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/toolkit/modules/FormLikeFactory.jsm#68
(In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are blocking you) from comment #8) > Gerv, we would like to use derivatives of real websites as test cases for > Form Autofill which I would hope is fine under fair use rules. Do we need to > do anything special to clarify the license e.g. that it doesn't fall under > the default PD license for tests? I think this needs to be a Legal bug. Feel free to CC me if you like, but real lawyers need to answer this question. Gerv
Flags: needinfo?(gerv)
Comment on attachment 8851202 [details] Bug 1349489 - Part 1: Move the codes from FormAutofillHandler.collectFormFields to FormAutofillHeuristics.getFormInfo.; https://reviewboard.mozilla.org/r/123570/#review132818 Hi Sean, Can you put the third-party websites in their own subdirectory (of `fixtures`) with a README containing: > This directory contains pages downloaded from the web for the purpose of testing Form Autofill against pages from the real world. These files are not made available under an open source license. as it sounds like that is what will be needed for the legal bug.
Attachment #8851202 - Flags: review?(MattN+bmo)
Hey MattN, The latest patch includes the refined test_fixtures.js which is very close to the final version. We just uncomment the related cases when any new predictions are implemented, so the test pattern can be improved in other bugs easily if we found any cases should be fixed. BTW, bug 1349490 provides the regexp solutions to fulfill the most cases.
Priority: -- → P1
Comment on attachment 8851202 [details] Bug 1349489 - Part 1: Move the codes from FormAutofillHandler.collectFormFields to FormAutofillHeuristics.getFormInfo.; https://reviewboard.mozilla.org/r/123570/#review135130 ::: commit-message-1b929:1 (Diff revision 11) > +Bug 1349489 - Add the test fixtures for the form field prediction.; r?MattN Nit: "Add test fixtures for autofill field name heuristics" ::: browser/extensions/formautofill/FormAutofillHandler.jsm:65 (Diff revision 11) > /** > * Set fieldDetails from the form about fields that can be autofilled. > */ > collectFormFields() { > - this.fieldDetails = []; > - > + let fieldDetails = FormAutofillHeuristics.getFormInfo(this.form); > + this.fieldDetails = fieldDetails ? fieldDetails : []; The changes to shipping JSMs really should be in a different commit/bug. ::: browser/extensions/formautofill/test/fixtures/autocomplete_basic.html:3 (Diff revision 11) > +<head> > + <title>Form Autofill Demo Page</title> I think this will give a console warning due to a lack of <meta charset="utf-8"> above <title> ::: browser/extensions/formautofill/test/fixtures/third_party/README:1 (Diff revision 11) > + This directory contains pages downloaded from the web for the purpose of Nit: it looks like you have a space at the beginning ::: browser/extensions/formautofill/test/unit/test_fixtures.js:8 (Diff revision 11) > +const TEST_PATTERNS = [ > + { > + fixturePath: "third_party/BestBuy/Checkout_ShippingAddress.html", > + expectedResult: [ > + [], // Search form > + [ I think it would be more maintainable with each entry in TEST_PATTERNS or each site in its own test file (in the appropriate site subdirectory): 1) It would mean that one intermittent issue wouldn't cause all of these cases to be disabled. 2) It would make it easier to track intermittents for the different fixtures. 3) It keeps VCS history simpler 4) Just generally feels more manageable. You can share the logic with head files. For example, the BestBuy test .js file(s) can be in the BestBuy directory, either one per .html file or one for all 3.
Attachment #8851202 - Flags: review?(MattN+bmo)
Comment on attachment 8851202 [details] Bug 1349489 - Part 1: Move the codes from FormAutofillHandler.collectFormFields to FormAutofillHeuristics.getFormInfo.; https://reviewboard.mozilla.org/r/123570/#review135130 > I think it would be more maintainable with each entry in TEST_PATTERNS or each site in its own test file (in the appropriate site subdirectory): > 1) It would mean that one intermittent issue wouldn't cause all of these cases to be disabled. > 2) It would make it easier to track intermittents for the different fixtures. > 3) It keeps VCS history simpler > 4) Just generally feels more manageable. > > You can share the logic with head files. > > For example, the BestBuy test .js file(s) can be in the BestBuy directory, either one per .html file or one for all 3. I think the test fixtures can be shared with mochitest too, so these fixtures are placed in `browser/extensions/formautofill/test/fixtures/`. However, I can not find a way to put the xpcshell-test files in that folder too. All the test scripts are in `browser/extensions/formautofill/test/unit/heuristics/`.
Comment on attachment 8851202 [details] Bug 1349489 - Part 1: Move the codes from FormAutofillHandler.collectFormFields to FormAutofillHeuristics.getFormInfo.; https://reviewboard.mozilla.org/r/123570/#review137078
Attachment #8851202 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8860805 [details] Bug 1349489 - Part 2: Add test fixtures for autofill field name heuristics.; https://reviewboard.mozilla.org/r/132770/#review137080 Thanks ::: browser/extensions/formautofill/test/unit/head.js:106 (Diff revision 1) > + > + Assert.equal(forms.length, testPattern.expectedResult.length, "Expected form count."); > + > + forms.forEach((form, formIndex) => { > + let formInfo = FormAutofillHeuristics.getFormInfo(form); > + // TODO: This line should be uncommnet to make sure every field are verified. Typo: "uncommented"
Attachment #8860805 - Flags: review?(MattN+bmo) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/4ff3acec9515 Part 1: Move the codes from FormAutofillHandler.collectFormFields to FormAutofillHeuristics.getFormInfo.; r=MattN https://hg.mozilla.org/integration/autoland/rev/6208b116de63 Part 2: Add test fixtures for autofill field name heuristics.; r=MattN
Keywords: checkin-needed
Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e92b30d30768 Backed out changeset 6208b116de63 for eslint failure https://hg.mozilla.org/integration/autoland/rev/cc35b73d4f98 Backed out changeset 4ff3acec9515 . a=backout
Flags: needinfo?(selee)
Keywords: checkin-needed
Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a093b634c4d5 Part 1: Move the codes from FormAutofillHandler.collectFormFields to FormAutofillHeuristics.getFormInfo.; r=MattN https://hg.mozilla.org/integration/autoland/rev/7b8379154e08 Part 2: Add test fixtures for autofill field name heuristics.; r=MattN
Keywords: checkin-needed
Oh, yesterday's backout didn't mention that you also had xpcshell failures? Backed out in https://hg.mozilla.org/integration/autoland/rev/d93f236ff5f5 for https://treeherder.mozilla.org/logviewer.html#?job_id=95020663&repo=autoland (every platform, that Mac one was just handy).
Since the codes in m-c affects the result of xpcshell-test, the test has been fixed and wait for try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba72c5db986f8e41d7824096dc0a76a5ac3bc6b0 Once it's done, I will push the patch again. Thanks.
Update the patch with try server green. (see comment 35)
Keywords: checkin-needed
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/6b3ebb0930bd Part 1: Move the codes from FormAutofillHandler.collectFormFields to FormAutofillHeuristics.getFormInfo.; r=MattN https://hg.mozilla.org/integration/autoland/rev/e7ace7047297 Part 2: Add test fixtures for autofill field name heuristics.; r=MattN
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by philringnalda@gmail.com: https://hg.mozilla.org/integration/autoland/rev/89db6edfc8be Part 1: Move the codes from FormAutofillHandler.collectFormFields to FormAutofillHeuristics.getFormInfo.; r=MattN https://hg.mozilla.org/integration/autoland/rev/2379d84a2855 Part 2: Add test fixtures for autofill field name heuristics.; r=MattN
Keywords: checkin-needed
FormAutofillHeuristics module is changed again since last rebasing. This is the same cause with comment 35 but test case slightly different. Thanks for your patient to land the patches.
https://hg.mozilla.org/mozilla-central/rev/89db6edfc8be https://hg.mozilla.org/mozilla-central/rev/2379d84a2855
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: qe-verify-
See Also: → 1378694
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: