Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2897083002:
[credentialsmanager] Remove redundant tests (Closed)
Description[credentialsmanager] Remove redundant tests
These assertions have been implemented in the Web Platform Tests
project. There, they are expressed with the `testharness.js` library,
which is more thorough than the locally-defined `verify_interface`
utility function. Remove the Chromium-specific assertions in favor of
those in the shared tests, and remove the now-unused `verify_interface`
utility.
BUG=724212
R=ellyjones@chromium.org, mkwst@chromium.org
Review-Url: https://codereview.chromium.org/2897083002
Cr-Commit-Position: refs/heads/master@{#475519}
Committed: https://chromium.googlesource.com/chromium/src/+/e11116938aaefd39902c8441f25bd3e230e9c487
Patch Set 1 #Patch Set 2 : Update expectations files #
Total comments: 9
Patch Set 3 : Remove redundant assertions #
Messages
Total messages: 30 (14 generated)
Hi Elly, mind taking a look?
Description was changed from ========== [credntialsmanager] Use shared tooling in tests Replace references to the local `verify_interface` function with the more thorough `idlharness.js` utility. Modify tests to assert the relevant WebIDL in its entirety, and update "expectations" files to reflect the implementation bugs this change demonstrates. BUG=724212 R=ellyjones@chromium.org ========== to ========== [credentialsmanager] Use shared tooling in tests Replace references to the local `verify_interface` function with the more thorough `idlharness.js` utility. Modify tests to assert the relevant WebIDL in its entirety, and update "expectations" files to reflect the implementation bugs this change demonstrates. BUG=724212 R=ellyjones@chromium.org ==========
lgtm
The CQ bit was checked by mike@mikepennisi.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/05/23 19:23:47, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) This patch failed the try jobs, but I'm having trouble finding relevant information from all the logging data available. Could you point me in the right direction, Elly?
On 2017/05/24 18:02:44, mike3 wrote: > On 2017/05/23 19:23:47, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) > > This patch failed the try jobs, but I'm having trouble finding relevant > information from all the logging data available. Could you point me in the > right direction, Elly? The try job output takes a minute or two to load from luci but after that you should get the entire stdout from the layout tests, which I hope contains what you need? I do not work on blink ever so I am not well-suited to help debug this :)
On 2017/05/25 18:42:32, Elly Fong-Jones wrote: > On 2017/05/24 18:02:44, mike3 wrote: > > On 2017/05/23 19:23:47, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) > > > > This patch failed the try jobs, but I'm having trouble finding relevant > > information from all the logging data available. Could you point me in the > > right direction, Elly? > > The try job output takes a minute or two to load from luci but after that you > should get the entire stdout from the layout tests, which I hope contains what > you need? I do not work on blink ever so I am not well-suited to help debug this > :) I've been digging through those files, but I still haven't found any conclusive explanation for the failure. I doubt my confused stumbling over the logs will be useful to anyone else, but I'll include them here in case it helps anyone explain what I'm missing. I should start by saying that I re-built Chromium on this past Monday (although I used a commit from Friday--fa112ee5b220b30022bc27d68f3eeed618e9f154). The tests pass for me locally, both when visited in a head-ful browser instance manually, and when run via `run-webkit-tests`. When it comes to the reported results, all links labeled "cache" require logging in, and after doing so, they all return an HTTP 404 response for me. I am able to access all the other referenced files, though; it seems as though the log labeled "[all]" is the most comprehensive. The first references to the failing tests indicate a test failure. For instance: 12:08:55.224 11240 [552/56529] virtual/mojo-loading/http/tests/credentialmanager/credentialscontainer-get-basics.html failed unexpectedly (text diff) 12:08:55.218 11372 worker/1 virtual/mojo-loading/http/tests/credentialmanager/credentialscontainer-get-basics.html failed: 12:08:55.218 11372 worker/1 text diff (The surrounding lines concern unrelated workers/files.) This makes me think that there is some problem with the `-expected.txt` files, but I'm not sure what that could be since no diff seems to be rendered. I generated these files using `run-webkit-tests --reset-results`, so I don't think they contain transcription errors like typos or whitespace differences. I've removed those files locally and re-run with `--reset-results`, and the newly-generated files match. A little later in the logs, the tests are re-tried. The output there is somewhat different: 12:47:46.432 26186 worker/0 http/tests/credentialmanager/credentialscontainer-get-basics.html output stderr lines: 12:47:46.432 26186 Xlib: extension "RANDR" missing on display ":99". 12:47:46.437 11240 [1/44] http/tests/credentialmanager/credentialscontainer-get-basics.html failed unexpectedly (text diff) 12:47:46.433 26186 worker/0 http/tests/credentialmanager/credentialscontainer-get-basics.html failed: 12:47:46.434 26186 worker/0 text diff That seems like an xvfb issue, which would suggest some deeper problem with the testing infrastructure. Since this patch only modifies HTML, JavaScript, and test expectation files, I don't think it could influence the infrastructure in any meaningful way (beyond maybe timing out, as suggested by later output). This is followed by a sort of summary: Regressions: Unexpected text-only failures (4) http/tests/credentialmanager/credentialscontainer-get-basics.html [ Failure ] http/tests/credentialmanager/passwordcredential-basics.html [ Failure ] virtual/mojo-loading/http/tests/credentialmanager/credentialscontainer-get-basics.html [ Failure ] virtual/mojo-loading/http/tests/credentialmanager/passwordcredential-basics.html [ Failure ] Regressions: Unexpected crashes (1) editing/shadow/doubleclick-on-meter-in-shadow-crash.html [ Crash ] The crashing test is completely unrelated to the patch, so I think it's an intermittent problem. This is followed by numerous references in output from a "dashboard rendering" process. I don't think these are related. The final references are part of a summary: 12:57:56.299 20325 10 slowest tests that are not marked as SLOW and did not timeout/crash: 12:57:56.299 20325 virtual/mojo-loading/http/tests/credentialmanager/passwordcredential-basics.html took 0.2 seconds 12:57:56.299 20325 http/tests/credentialmanager/credentialscontainer-get-basics.html took 0.2 seconds 12:57:56.299 20325 virtual/mojo-loading/http/tests/credentialmanager/credentialscontainer-get-basics.html took 0.1 seconds 12:57:56.299 20325 http/tests/credentialmanager/passwordcredential-basics.html took 0.1 seconds 12:57:56.299 20325 12:57:56.299 20325 Tests that timed out or crashed: 12:57:56.299 20325 editing/shadow/doubleclick-on-meter-in-shadow-crash.html took 0.6 seconds 12:57:56.299 20325 12:57:56.299 20325 12:57:56.299 20325 4 tests ran as expected, 1 didn't: 12:57:56.299 20325 => Results: 4/5 tests passed (80.0%) => Tests to be fixed (1): 1 crashes (100.0%) => Tests that will only be fixed if they crash (WONTFIX) (0): Regressions: Unexpected crashes (1) editing/shadow/doubleclick-on-meter-in-shadow-crash.html [ Crash ] The above suggests that the relevant test files are running too slowly. I don't understand why that would be the case--`idlharness.js` is not particularly computation-heavy. Then again, it's not clear that this is actually a problem, since "4 tests ran as expected." Those are all the references I was able to find, but I'm still not sure how to proceed. I suspect I'm missing some more helpful link in the UI, though. I'll try asking around in the #chromium IRC channel tomorrow.
I got a tip from falken@ over in https://codereview.chromium.org/2897113002/. Now I can see the problem: a commit landed in the time since I last built Chromium which introduced a new deprecation warning in the developer console. I've updated the "expectations" files accordingly; re-starting the build now.
mike@mikepennisi.com changed reviewers: + jdoerrie@chromium.org
On second thought, since this patch was effected by a change recently made by jdoerrie, I'd like to get their input. Would you mind taking a look, jdoerrie?
jdoerrie@chromium.org changed reviewers: + mkwst@chromium.org
While it is true that I recently changed these files, I have only very basic knowledge of IDL tests. Therefore I'm adding mkwst@chromium.org to have a look at this.
Thank you for this patch! I think we can make it even smaller by just removing these tests, as they seem to entirely replicate coverage we've upstreamed to WPT. Comments inline. https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-basics.html (right): https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-basics.html:20: idl_array.test(); I think we can just delete this test entirely. It should be subsumed by https://github.com/w3c/web-platform-tests/blob/master/credential-management/i... (though I note that that test is missing the `[Exposed=Window, SecureContext]` bit from the interface; we should fix that). https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-basics.html (right): https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-basics.html:163: idl_array.test(); Likewise, the IDL checks here can just be removed in favor of those already in https://github.com/w3c/web-platform-tests/blob/master/credential-management/i.... https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/credentialmanager/federatedcredential-basics.html (right): https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/credentialmanager/federatedcredential-basics.html:48: idl_array.test(); Likewise. https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html (right): https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html:45: idl_array.test(); Likewise.
Description was changed from ========== [credentialsmanager] Use shared tooling in tests Replace references to the local `verify_interface` function with the more thorough `idlharness.js` utility. Modify tests to assert the relevant WebIDL in its entirety, and update "expectations" files to reflect the implementation bugs this change demonstrates. BUG=724212 R=ellyjones@chromium.org ========== to ========== [credentialsmanager] Remove redundant tests These assertions have been implemented in the Web Platform Tests project. There, they are expressed with the `testharness.js` library, which is more thorough than the locally-defined `verify_interface` utility function. Remove the Chromium-specific assertions in favor of those in the shared tests, and remove the now-unused `verify_interface` utility. BUG=724212 R=ellyjones@chromium.org, mkwst@chromium.org ==========
Thanks for the review! I've removed the assertions as requested and updated the CL description accordingly. This is now failing for me locally, but I believe that is because my build is out of date. I can attempt to re-build and validate the patch locally if you would like, but building takes many hours on my system. Do you think we could let the CQ validate on my behalf? https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-basics.html (right): https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-basics.html:20: idl_array.test(); On 2017/05/29 12:10:38, Mike West wrote: > I think we can just delete this test entirely. It should be subsumed by > https://github.com/w3c/web-platform-tests/blob/master/credential-management/i... > (though I note that that test is missing the `[Exposed=Window, SecureContext]` > bit from the interface; we should fix that). Done. https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-basics.html (right): https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-basics.html:163: idl_array.test(); On 2017/05/29 12:10:38, Mike West wrote: > Likewise, the IDL checks here can just be removed in favor of those already in > https://github.com/w3c/web-platform-tests/blob/master/credential-management/i.... Done. https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/credentialmanager/federatedcredential-basics.html (right): https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/credentialmanager/federatedcredential-basics.html:48: idl_array.test(); On 2017/05/29 12:10:38, Mike West wrote: > Likewise. Done. https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html (right): https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html:45: idl_array.test(); On 2017/05/29 12:10:38, Mike West wrote: > Likewise. Done.
https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-basics.html (right): https://codereview.chromium.org/2897083002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-basics.html:20: idl_array.test(); On 2017/05/29 12:10:38, Mike West wrote: > I think we can just delete this test entirely. It should be subsumed by > https://github.com/w3c/web-platform-tests/blob/master/credential-management/i... > (though I note that that test is missing the `[Exposed=Window, SecureContext]` > bit from the interface; we should fix that). Follow-up in WPT: https://github.com/w3c/web-platform-tests/pull/6076
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mike@mikepennisi.com
The patchset sent to the CQ was uploaded after l-g-t-m from ellyjones@chromium.org Link to the patchset: https://codereview.chromium.org/2897083002/#ps40001 (title: "Remove redundant assertions")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496153823479110,
"parent_rev": "cf9d12f1999e34f40e9e3ee9e40829d4529cb55b", "commit_rev":
"e11116938aaefd39902c8441f25bd3e230e9c487"}
Message was sent while issue was closed.
Description was changed from ========== [credentialsmanager] Remove redundant tests These assertions have been implemented in the Web Platform Tests project. There, they are expressed with the `testharness.js` library, which is more thorough than the locally-defined `verify_interface` utility function. Remove the Chromium-specific assertions in favor of those in the shared tests, and remove the now-unused `verify_interface` utility. BUG=724212 R=ellyjones@chromium.org, mkwst@chromium.org ========== to ========== [credentialsmanager] Remove redundant tests These assertions have been implemented in the Web Platform Tests project. There, they are expressed with the `testharness.js` library, which is more thorough than the locally-defined `verify_interface` utility function. Remove the Chromium-specific assertions in favor of those in the shared tests, and remove the now-unused `verify_interface` utility. BUG=724212 R=ellyjones@chromium.org, mkwst@chromium.org Review-Url: https://codereview.chromium.org/2897083002 Cr-Commit-Position: refs/heads/master@{#475519} Committed: https://chromium.googlesource.com/chromium/src/+/e11116938aaefd39902c8441f25b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e11116938aaefd39902c8441f25b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
