Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added maplike and setlike handling in idlharness.js #9878

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@ghost
Copy link

ghost commented Mar 6, 2018

This change is Reviewable

@wpt-pr-bot wpt-pr-bot added the infra label Mar 6, 2018

@wpt-pr-bot wpt-pr-bot requested review from ayg, gsnedders and jgraham Mar 6, 2018

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Mar 6, 2018

issue #8616
I have declared an add_maplike_members function and called that function in a case "maplike" in test_members

@gsnedders gsnedders requested review from tobie and foolip Mar 6, 2018

@jgraham
Copy link
Contributor

jgraham left a comment

I think this looks reasonable, thanks! But I would like @tobie or @foolip to take another look.

case "maplike":
this.add_maplike_members(member);
break;
// TODO: add setlike and maplike handling.

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 6, 2018

Contributor

This comment is now wrong.

This comment has been minimized.

Copy link
@ghost

ghost Mar 7, 2018

Author

OK I will change.May I know whether the implementation of maplike is correct?

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 7, 2018

Build PASSED

Started: 2018-03-29 12:42:38
Finished: 2018-03-29 12:48:39

View more information about this build on:

@ghost ghost changed the title Added maplike handling by declaring a function Added maplike and setlike handling in idlharness.js Mar 7, 2018

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Mar 23, 2018

Can I know how to get over the failure case?

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 23, 2018

@manikishan, we've had some trouble with resources_unittest lately, to rule out those problem can you rebase your changes on master and push, so that Travis will run again?

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 23, 2018

Unfortunately, with our current Travis setup it's very hard to know what the consequence of a change like this across all tests is going to be. I'll try running it through Chromium's CQ to see which tests will be affected.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Mar 23, 2018

@foolip Thanks, I will rebase and push it again

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 23, 2018

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 23, 2018

Results are in https://test-results.appspot.com/data/layout_results/linux_chromium_rel_ng/52585/layout-test-results/results.html

Looks like all idlharness.js tests are broken. Can you test your changes with some of the tests locally? When you think it's in good shape, I can try it again.

this.members.push(new IdlInterfaceMember(
{ type: "operation", name: "size", idlType: "maplike", arguments: []}));
this.members.push(new IdlInterfaceMember(
{ type: "operation", name: "entries", idlType: "maplike", arguments []}));

This comment has been minimized.

Copy link
@foolip

foolip Mar 23, 2018

Contributor

The problem is a missing : here and elsewhere.

This comment has been minimized.

Copy link
@ghost

ghost Mar 24, 2018

Author

I will change it soon. Thank you!

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 26, 2018

I'll try it again.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 26, 2018

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 26, 2018

Results are in and it looks like only a single test is affected, webaudio/idlharness.https.html:
https://test-results.appspot.com/data/layout_results/linux_chromium_rel_ng/53647/layout-test-results/results.html

That's less than I expected. @manikishan, interfaces/webrtc-pc.idl has "maplike" and interfaces/css-font-loading.idl has "setlike". Can you check the tests involving those to see if the results are what you expected?

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Mar 26, 2018

@foolip , Sure I will Thank you!

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Mar 26, 2018

Can I please know which tests I have to run to check the interfaces? @foolip

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 27, 2018

@manikishan, these are the files and how I found them:

$ git grep -l webrtc-pc.idl
webrtc/interfaces.https.html
$ git grep -l font-loading.idl
css/css-font-loading/idlharness.https.html
@ghost

This comment has been minimized.

Copy link
Author

ghost commented Mar 27, 2018

Yes, I found them too. I was meant to ask how could I check it's implementation. I think I have to run some tests and if yes, what tests should I run? Sorry if I misunderstood anything

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 28, 2018

@manikishan, with your changes, has the total number of tests increased? And are the new tests passing or failing as you would expect them to? I ask because with iterable it turned out that there was something a bit wrong: #9790

Still haven't figured out exactly how to fix that, but if there are similar issues with maplike and setlike it'd be good to know before they show up as incorrectly failing tests.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Mar 29, 2018

@foolip I have run the tests in the local repository and found two failures .

assert_equals: expected (undefined) undefined but got (function) function "function DOMError() { [native code] }"

isInterfaceNuked/<@http://web-platform.test:8000/dom/historical.html:10:5
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1501:20
test@http://web-platform.test:8000/resources/testharness.js:511:9
isInterfaceNuked@http://web-platform.test:8000/dom/historical.html:9:3
@http://web-platform.test:8000/dom/historical.html:32:1
assert_equals: expected (undefined) undefined but got (boolean) true

isNukedFromDocument/<@http://web-platform.test:8000/dom/historical.html:38:5
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1501:20
test@http://web-platform.test:8000/resources/testharness.js:511:9
isNukedFromDocument@http://web-platform.test:8000/dom/historical.html:35:3
@http://web-platform.test:8000/dom/historical.html:60:1

Does these failures give any hint to us?

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 29, 2018

@manikishan, it looks like you were running dom/historical.html instead of webrtc/interfaces.https.html and css/css-font-loading/idlharness.https.html.

In the affected tests, are the new failing subtests? You can try it in any browser you think supports the maplike and setlike bits of these interfaces. If none do, then just check to see if the failures are as you would expect them to be, and not failures because of some syntax error or similar.

@ghost

This comment has been minimized.

Copy link
Author

ghost commented Mar 29, 2018

@foolip I tried to run tests on webrtc/interfaces.https.html and css/css-font-loading/idlharness.https.html The result was an error stating no tests were run. From this can we get any information regarding the error?

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented May 9, 2018

@manikishan did that error happen before your changes as well? If not, you'll need to figure out what in your changes broke the tests. Sorry to say that tooling for detecting regressions doesn't exist yet, so this is all time-consuming and frustrating for everyone :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.