1481295 - Enable PaymentRequest in WPT /payment-request/ tests

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement, P1)

Description

[Blake Kaplan (:mrbkap) (inactive)]

I noticed looking at another bug that we don't set the pref for idlharness.https.window.js.ini, so it fails. It seems like we might as run the test with the pref set.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1481295 - Set the pref so we see what's actually failing.

Comment 2

[Eden Chuang[:edenchuang]]

Comment on attachment 8998006 [details]
Bug 1481295 - Set the pref so we see what's actually failing.

Eden Chuang[:edenchuang] has approved the revision.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5873bdd1241
Set the pref so we see what's actually failing. r=edenchuang

Comment 4

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset f5873bdd1241 (Bug 1481295) for idlharness.https.window.html failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f5873bdd1241fc05a1839e78ac1b24774eb7c58a

Backout link: https://hg.mozilla.org/integration/autoland/rev/81b7c0a6c98663406ab8321f9f686837761dfbae

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=192804564&repo=autoland&lineNumber=18598
[task 2018-08-08T16:06:38.504Z] 16:06:38     INFO - TEST-START | /payment-request/idlharness.https.window.html
[task 2018-08-08T16:06:38.605Z] 16:06:38     INFO - Setting pref dom.payments.request.enabled (true)
[task 2018-08-08T16:06:39.540Z] 16:06:39     INFO - PID 8682 | ++DOCSHELL 0xe043f000 == 9 [pid = 8682] [id = {b96de763-6e8c-45b4-910c-bfbf5274b869}]
[task 2018-08-08T16:06:39.540Z] 16:06:39     INFO - PID 8682 | ++DOMWINDOW == 27 (0xdfe94160) [pid = 8682] [serial = 27] [outer = (nil)]
[task 2018-08-08T16:06:39.556Z] 16:06:39     INFO - PID 8682 | ++DOMWINDOW == 28 (0xe0440c00) [pid = 8682] [serial = 28] [outer = 0xdfe94160]
[task 2018-08-08T16:06:39.653Z] 16:06:39     INFO - PID 8682 | ++DOMWINDOW == 29 (0xe043fc00) [pid = 8682] [serial = 29] [outer = 0xdfe94160]
[task 2018-08-08T16:06:39.762Z] 16:06:39     INFO - PID 8682 | [8682, Main Thread] WARNING: Failed to retarget HTML data delivery to the parser thread.: file /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp, line 1015
[task 2018-08-08T16:06:41.582Z] 16:06:41     INFO - 
[task 2018-08-08T16:06:41.582Z] 16:06:41     INFO - TEST-PASS | /payment-request/idlharness.https.window.html | Setup for Payment Request API IDL tests. 
[task 2018-08-08T16:06:41.583Z] 16:06:41     INFO - TEST-UNEXPECTED-FAIL | /payment-request/idlharness.https.window.html | PaymentRequest interface: existence and properties of interface object - assert_own_property: self does not have own property "PaymentRequest" expected property "PaymentRequest" missing
[task 2018-08-08T16:06:41.585Z] 16:06:41     INFO - IdlInterface.prototype.test_self/<@https://web-platform.test:8443/resources/idlharness.js:1506:9
[task 2018-08-08T16:06:41.586Z] 16:06:41     INFO - Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1538:20
[task 2018-08-08T16:06:41.588Z] 16:06:41     INFO - test@https://web-platform.test:8443/resources/testharness.js:545:9
[task 2018-08-08T16:06:41.590Z] 16:06:41     INFO - self.subsetTestByKey@https://web-platform.test:8443/resources/idlharness.js:54:23
[task 2018-08-08T16:06:41.591Z] 16:06:41     INFO - IdlInterface.prototype.test_self@https://web-platform.test:8443/resources/idlharness.js:1486:5
[task 2018-08-08T16:06:41.593Z] 16:06:41     INFO - IdlInterface.prototype.test@https://web-platform.test:8443/resources/idlharness.js:1470:9
[task 2018-08-08T16:06:41.595Z] 16:06:41     INFO - IdlArray.prototype.test@https://web-platform.test:8443/resources/idlharness.js:857:9
[task 2018-08-08T16:06:41.597Z] 16:06:41     INFO - idl_test/</<@https://web-platform.test:8443/resources/idlharness.js:3226:21
[task 2018-08-08T16:06:41.599Z] 16:06:41     INFO - Async*idl_test/<@https://web-platform.test:8443/resources/idlharness.js:3205:16
[task 2018-08-08T16:06:41.600Z] 16:06:41     INFO - Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1538:20
[task 2018-08-08T16:06:41.601Z] 16:06:41     INFO - promise_test/tests.promise_tests<@https://web-platform.test:8443/resources/testharness.js:577:27
[task 2018-08-08T16:06:41.602Z] 16:06:41     INFO - promise callback*promise_test@https://web-platform.test:8443/resources/testharness.js:573:31
[task 2018-08-08T16:06:41.602Z] 16:06:41     INFO - idl_test@https://web-platform.test:8443/resources/idlharness.js:3200:12
[task 2018-08-08T16:06:41.602Z] 16:06:41     INFO - @https://web-platform.test:8443/payment-request/idlharness.https.window.js:8:1
[task 2018-08-08T16:06:41.603Z] 16:06:41     INFO - 
[task 2018-08-08T16:06:41.603Z] 16:06:41     INFO - TEST-UNEXPECTED-FAIL | /payment-request/idlharness.https.window.html | PaymentRequest interface object length - assert_own_property: self does not have own property "PaymentRequest" expected property "PaymentRequest" missing

Comment 5

[Marion Daly [:mdaly]]

:asuth on your radar

Comment 7

[Marco Mucci [:MarcoM]]

For assigned bugs being actively worked on we put the priority at P1.

Comment 8

[Eden Chuang[:edenchuang]]

Attachment: Set Pref dom.payments.request.enabled on for running wp-tests with expected Pass results on tryserver.

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 9012191 [details] [diff] [review]
Set Pref dom.payments.request.enabled on for running wp-tests with expected Pass results on tryserver.

Review of attachment 9012191 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/meta/payment-request/MerchantValidationEvent/complete-method.https.html.ini
@@ +1,1 @@
>  [complete-method.https.html]

Remove this file completely.

::: testing/web-platform/meta/payment-request/MerchantValidationEvent/constructor.https.html.ini
@@ +1,1 @@
>  [constructor.https.html]

All of these files must be removed.

Comment 10

[Marcos Caceres [:marcosc]]

Comment on attachment 9012191 [details] [diff] [review]
Set Pref dom.payments.request.enabled on for running wp-tests with expected Pass results on tryserver.

Review of attachment 9012191 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this Eden. I'm super excited that all these will finally be enabled! 

Just two small things related to "dom.payments.request.user_interaction_required".

::: testing/web-platform/meta/payment-request/payment-request-abort-method.https.html.ini
@@ +1,2 @@
>  [payment-request-abort-method.https.html]
> +  prefs: [dom.payments.request.user_interaction_required:false]

Please remove this, or leave it as expect FAIL. That will remind us to fix this later.

::: testing/web-platform/meta/payment-request/payment-request-canmakepayment-method.https.html.ini
@@ +1,2 @@
>  [payment-request-canmakepayment-method.https.html]
> +  prefs: [dom.payments.request.user_interaction_required:false]

Same as the other. Let this one FAIL.

Comment 11

[Marcos Caceres [:marcosc]]

Comment on attachment 9012191 [details] [diff] [review]
Set Pref dom.payments.request.enabled on for running wp-tests with expected Pass results on tryserver.

Review of attachment 9012191 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/meta/payment-request/payment-response/onpayerdetailchange-attribute.manual.https.html.ini
@@ +1,2 @@
>  [onpayerdetailchange-attribute.manual.https.html]
> +  expected: TIMEOUT

Note, I fixed this this in https://bugzilla.mozilla.org/show_bug.cgi?id=1493110

The file was incorrectly named. I think this filed can be deleted (or probably has been deleted upstream). The new file should not timeout anymore :)

Comment 12

[Marcos Caceres [:marcosc]]

Comment on attachment 9012191 [details] [diff] [review]
Set Pref dom.payments.request.enabled on for running wp-tests with expected Pass results on tryserver.

Review of attachment 9012191 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/meta/payment-request/payment-response/onpayerdetailchange-attribute.manual.https.html.ini
@@ +1,2 @@
>  [onpayerdetailchange-attribute.manual.https.html]
> +  expected: TIMEOUT

Note, I fixed this this in https://bugzilla.mozilla.org/show_bug.cgi?id=1493110

The file was incorrectly named. I think this filed can be deleted (or probably has been deleted upstream). The new file should not timeout anymore :)

Comment 13

[Eden Chuang[:edenchuang]]

Attachment: Set Pref dom.payments.request.enabled on for running wp-tests with expected Pass results on tryserver.

update patch according to comment 9 and 10.

Comment 14

[Eden Chuang[:edenchuang]]

(In reply to Marcos Caceres [:marcosc] from comment #10)
> Comment on attachment 9012191 [details] [diff] [review]
> Set Pref dom.payments.request.enabled on for running wp-tests with expected
> Pass results on tryserver.
> 
> Review of attachment 9012191 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for doing this Eden. I'm super excited that all these will finally be
> enabled! 
> 
> Just two small things related to
> "dom.payments.request.user_interaction_required".
> 
> :::
> testing/web-platform/meta/payment-request/payment-request-abort-method.https.
> html.ini
> @@ +1,2 @@
> >  [payment-request-abort-method.https.html]
> > +  prefs: [dom.payments.request.user_interaction_required:false]
> 
> Please remove this, or leave it as expect FAIL. That will remind us to fix
> this later.
> 
> :::
> testing/web-platform/meta/payment-request/payment-request-canmakepayment-
> method.https.html.ini
> @@ +1,2 @@
> >  [payment-request-canmakepayment-method.https.html]
> > +  prefs: [dom.payments.request.user_interaction_required:false]
> 
> Same as the other. Let this one FAIL.

In fact, I file a following bug for it. https://bugzilla.mozilla.org/show_bug.cgi?id=1494340
but I still update the patch according to your comment. Let tests be expected FAIL.

Comment 15

[Eden Chuang[:edenchuang]]

(In reply to Marcos Caceres [:marcosc] from comment #12)
> Comment on attachment 9012191 [details] [diff] [review]
> Set Pref dom.payments.request.enabled on for running wp-tests with expected
> Pass results on tryserver.
> 
> Review of attachment 9012191 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> testing/web-platform/meta/payment-request/payment-response/
> onpayerdetailchange-attribute.manual.https.html.ini
> @@ +1,2 @@
> >  [onpayerdetailchange-attribute.manual.https.html]
> > +  expected: TIMEOUT
> 
> Note, I fixed this this in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1493110
> 
> The file was incorrectly named. I think this filed can be deleted (or
> probably has been deleted upstream). The new file should not timeout anymore
> :)

But I still get TIMEOUT on tryserver. I will double check on tryserver before I set checkin-needed.
I guess we should get the expected result, even the .ini file exists, because the test should not be run automatically.
So now, I will keep it on the patch, and if it still needed when I checkin, I will file a following bug for it.

Comment 16

[Marcos Caceres [:marcosc]]

(In reply to Eden Chuang[:edenchuang] from comment #15)
> But I still get TIMEOUT on tryserver. I will double check on tryserver
> before I set checkin-needed.

Yeah, that's odd. If it's the same file, then that's the problem. That file should have been deleted from upstream WPT. 

> I guess we should get the expected result, even the .ini file exists,
> because the test should not be run automatically.
> So now, I will keep it on the patch, and if it still needed when I checkin,
> I will file a following bug for it.

You can leave it as expecting TIMEOUT, I would guess... and it should self heal once the metadata gets regenerated.

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 9012510 [details] [diff] [review]
Set Pref dom.payments.request.enabled on for running wp-tests with expected Pass results on tryserver.

Review of attachment 9012510 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/meta/payment-request/payment-request-id-attribute.https.html.ini
@@ +2,3 @@
>    [PaymentRequest's id attribute must be a UUID when PaymentDetailsInit.id is missing]
> +    expected:
> +      if os == "mac": FAIL

why this? we need a comment a bug to fix the issue.

Comment 18

[Marcos Caceres [:marcosc]]

Comment on attachment 9012510 [details] [diff] [review]
Set Pref dom.payments.request.enabled on for running wp-tests with expected Pass results on tryserver.

Review of attachment 9012510 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/meta/payment-request/payment-request-id-attribute.https.html.ini
@@ +2,3 @@
>    [PaymentRequest's id attribute must be a UUID when PaymentDetailsInit.id is missing]
> +    expected:
> +      if os == "mac": FAIL

This is weird... the RegExp passes in all other browsers. We might need to get the JS folks to look at this?

Comment 19

[Marcos Caceres [:marcosc]]

To be clear, I confirmed that it's indeed failing in Mac OS - but I didn't check other OSs.

Comment 20

[Eden Chuang[:edenchuang]]

(In reply to Marcos Caceres [:marcosc] from comment #19)
> To be clear, I confirmed that it's indeed failing in Mac OS - but I didn't
> check other OSs.

Yes only on MacOS, but pass in other OSs.
I would like to comment it on .ini and file a bug to figure it out.

Comment 21

[Marcos Caceres [:marcosc]]

Fixing on WTP side by making the RegExp a little bit less strict: 
https://github.com/web-platform-tests/wpt/pull/13258

Comment 22

[Eden Chuang[:edenchuang]]

Attachment: Set Pref dom.payments.request.enabled on for running wp-tests with expected Pass results on tryserver.

Comment 23

[Arthur Iakab [arthur_iakab]]

Could not land patch, received this error message when trying to apply the patch: "unable to find 'testing/web-platform/meta/payment-request/payment-response/onpayerdetailchange-attribute.manual.https.html.ini' for patching"

:edenchuang could you please take a look?

Comment 24

[Marcos Caceres [:marcosc]]

That’s the file that got renamed from .manual to -manual.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77168ec151a9
set prefs for PaymentRequest web-platform-tests. r=baku.

Comment 26

[Matthew N. [:MattN]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=997eca4e30b6609265fab92a6b6f8c0918abbee2

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d45a7a0f6f29
Follow-up to revert mistaken PaymentAddress.languageCode change. r=bustage

Comment 28

[Marcos Caceres [:marcosc]]

Just want to record what the actual issue is... when the UUID is generated on MacOS, the third component's (time_hi_and_version) is not returning what's expected by the UUID spec: the first character should be a digit between 1-4. It might just have been coincidence that we didn't hit this on other platforms.

Looking briefly at nsID.{cpp, h} [1,2], I don't see anything platform specific there - but [2] does not claim to be a fully conforming implementation of UUID. It states: "A "unique identifier". This is modeled after OSF DCE UUIDs." 

Should we need info someone about this - maybe about making us more conforming? The Payment spec requires UUID. However, not providing a fully conforming UUID should not be a big deal in practice (famous last words, specially if there are systems/databases that do validate UUIDs). 

[1] https://searchfox.org/mozilla-central/source/xpcom/base/nsID.cpp
[2] https://searchfox.org/mozilla-central/source/xpcom/base/nsID.h#17

Comment 29

[Matthew N. [:MattN]]

It might be good to reference your comment in bug 1494920.

Comment 30

[Cristian Brindusan [:cbrindusan]]

Backed out 2 changesets (Bug 1481295) for wpt failures in /payment-request/historical.https.html.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/db8d6f1519509e2609d70184aa70d0aadfd77c9f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=pending,running,success,testfailed,busted,exception&revision=d45a7a0f6f297eb90e475f36158508f605c021d4&selectedJob=202351276

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=202351276&repo=mozilla-inbound&lineNumber=8252

Comment 31

[Eden Chuang[:edenchuang]]

Attachment: Set Pref dom.payments.request.enabled on for running wp-tests with expected Pass results on tryserver.

Fix the test fail on PaymentAddress updating in web-platform-tests.

Comment 32

[Eden Chuang[:edenchuang]]

Tryserver result.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad0158ccb8258fe4c549ccc3f9046ab3a76c58b7

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d799420c9b97
set prefs for PaymentRequest web-platform-tests. r=baku.

Comment 34

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d799420c9b97