Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2863593003:
Add Origin Trial layout tests for IDL bindings (part 2) (Closed)
DescriptionAdd Origin Trial layout tests for IDL bindings (part 2)
A continuation from: https://codereview.chromium.org/2861673007/
This moves existing tests for enabling/disabling to be explicit
bindings tests. When testing changes for bindings, many of the
layout test files would fail, all for the same reason. Now, there
is separation for token handling vs bindings. That is, testing
that trials are enabled/disabled correctly, vs that IDL members
are exposed correctly for a trial.
As well, cleaned up the existing tests to use the helper methods
introduced in the previous CL. Two separate CLs to make it easier
to diff all changes.
BUG=695123
Review-Url: https://codereview.chromium.org/2863593003
Cr-Commit-Position: refs/heads/master@{#471944}
Committed: https://chromium.googlesource.com/chromium/src/+/bf4fc3116e4ddc3bc718b95c571a87c143211f0a
Patch Set 1 #Patch Set 2 : Use helper methods for all tests #
Total comments: 6
Patch Set 3 : Address comments #Patch Set 4 : Rebase #
Messages
Total messages: 31 (24 generated)
The CQ bit was checked by chasej@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
Description was changed from ========== Move more tests that are really for bindings BUG=695123 ========== to ========== A continuation from: https://codereview.chromium.org/2861673007/ This moves existing tests for enabling/disabling to be explicit bindings tests. When testing changes for bindings, many of the layout test files would fail, all for the same reason. Now, bindings tests are limited to the minimum layout test files. As well, cleaned up the existing tests to use the helper methods introduced in the previous CL. Two separate CLs to make it easier to diff all changes. BUG=695123 ==========
The CQ bit was checked by chasej@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...
chasej@chromium.org changed reviewers: + iclelland@chromium.org
iclelland, please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the delay on this -- I like the new organization; there might be a couple of test files that need to be updated along with this, but otherwise it looks great. https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js (left): https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:93: desc: 'Attribute should not exist on partial interface, with trial disabled', The partial interface bindings tests are now no longer being run on layout tests like LayoutTests/http/tests/origin_trials/sample-api-expired.html, sample-api-broken.html, sample-api-stolen.html. Should we add a call to expect_failure_bindings in those tests, or is that overkill (really, those tests are just for token handling)? https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:141: for (var i = 0; i < tests.length; ++i) { I don't think it matters that the worker tests are now run after the tests for the document, but have you checked to see if there was a reason that we were delaying them like this before? https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:178: test(() => { Similar to the comment above, this means that sample-api-enabled-header.php isn't running these tests for partial interface bindings. That one, I think, *should* have a call now to expect_success_bindings() to ensure that processing the token in the header rather than during parsing calls the appropriate functions to bind everything. Maybe the same for sample-api-script-added-before-access.html
On 2017/05/10 14:27:43, iclelland wrote: > Sorry for the delay on this -- I like the new organization; there might be a > couple of test files that need to be updated along with this, but otherwise it > looks great. > > https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js > (left): > > https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:93: > desc: 'Attribute should not exist on partial interface, with trial disabled', > The partial interface bindings tests are now no longer being run on layout tests > like LayoutTests/http/tests/origin_trials/sample-api-expired.html, > sample-api-broken.html, sample-api-stolen.html. Should we add a call to > expect_failure_bindings in those tests, or is that overkill (really, those tests > are just for token handling)? > > https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:141: > for (var i = 0; i < tests.length; ++i) { > I don't think it matters that the worker tests are now run after the tests for > the document, but have you checked to see if there was a reason that we were > delaying them like this before? > > https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:178: > test(() => { > Similar to the comment above, this means that sample-api-enabled-header.php > isn't running these tests for partial interface bindings. That one, I think, > *should* have a call now to expect_success_bindings() to ensure that processing > the token in the header rather than during parsing calls the appropriate > functions to bind everything. > > Maybe the same for sample-api-script-added-before-access.html Unless I misunderstood the reason for breaking it up this way -- re-reading the CL description. If removing the bindings tests from all of those files is the point of this, then this is all great.
Description was changed from ========== A continuation from: https://codereview.chromium.org/2861673007/ This moves existing tests for enabling/disabling to be explicit bindings tests. When testing changes for bindings, many of the layout test files would fail, all for the same reason. Now, bindings tests are limited to the minimum layout test files. As well, cleaned up the existing tests to use the helper methods introduced in the previous CL. Two separate CLs to make it easier to diff all changes. BUG=695123 ========== to ========== A continuation from: https://codereview.chromium.org/2861673007/ This moves existing tests for enabling/disabling to be explicit bindings tests. When testing changes for bindings, many of the layout test files would fail, all for the same reason. Now, there is separation for token handling vs bindings. That is, testing that trials are enabled/disabled correctly, vs that IDL members are exposed correctly for a trial. As well, cleaned up the existing tests to use the helper methods introduced in the previous CL. Two separate CLs to make it easier to diff all changes. BUG=695123 ==========
The CQ bit was checked by chasej@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...
iclelland, please take another look. https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js (left): https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:93: desc: 'Attribute should not exist on partial interface, with trial disabled', On 2017/05/10 14:27:42, iclelland wrote: > The partial interface bindings tests are now no longer being run on layout tests > like LayoutTests/http/tests/origin_trials/sample-api-expired.html, > sample-api-broken.html, sample-api-stolen.html. Should we add a call to > expect_failure_bindings in those tests, or is that overkill (really, those tests > are just for token handling)? It is intentional that the partial bindings tests are not being run for all the various disabled scenarios. I've tried to clarify in the CL description, but the idea is to separate token handling vs bindings tests. https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:141: for (var i = 0; i < tests.length; ++i) { On 2017/05/10 14:27:42, iclelland wrote: > I don't think it matters that the worker tests are now run after the tests for > the document, but have you checked to see if there was a reason that we were > delaying them like this before? I don't believe there was a good reason for delaying the worker tests. From past testing, I recall that the worker tests are run later, asynchronously. So, the ordering of the fetch_tests_from_worker() call is mostly irrelevant, because the tests will be executed after control leaves the method. https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:178: test(() => { On 2017/05/10 14:27:42, iclelland wrote: > Similar to the comment above, this means that sample-api-enabled-header.php > isn't running these tests for partial interface bindings. That one, I think, > *should* have a call now to expect_success_bindings() to ensure that processing > the token in the header rather than during parsing calls the appropriate > functions to bind everything. > > Maybe the same for sample-api-script-added-before-access.html Yes, agreed that it makes sense to run the bindings tests in those scenarios. Added to those files.
On 2017/05/11 19:43:00, chasej wrote: > iclelland, please take another look. > > https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js > (left): > > https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:93: > desc: 'Attribute should not exist on partial interface, with trial disabled', > On 2017/05/10 14:27:42, iclelland wrote: > > The partial interface bindings tests are now no longer being run on layout > tests > > like LayoutTests/http/tests/origin_trials/sample-api-expired.html, > > sample-api-broken.html, sample-api-stolen.html. Should we add a call to > > expect_failure_bindings in those tests, or is that overkill (really, those > tests > > are just for token handling)? > > It is intentional that the partial bindings tests are not being run > for all the various disabled scenarios. I've tried to clarify in the > CL description, but the idea is to separate token handling vs bindings > tests. > > https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:141: > for (var i = 0; i < tests.length; ++i) { > On 2017/05/10 14:27:42, iclelland wrote: > > I don't think it matters that the worker tests are now run after the tests for > > the document, but have you checked to see if there was a reason that we were > > delaying them like this before? > > I don't believe there was a good reason for delaying the worker > tests. From past testing, I recall that the worker tests are run > later, asynchronously. So, the ordering of the fetch_tests_from_worker() > call is mostly irrelevant, because the tests will be executed after > control leaves the method. > > https://codereview.chromium.org/2863593003/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:178: > test(() => { > On 2017/05/10 14:27:42, iclelland wrote: > > Similar to the comment above, this means that sample-api-enabled-header.php > > isn't running these tests for partial interface bindings. That one, I think, > > *should* have a call now to expect_success_bindings() to ensure that > processing > > the token in the header rather than during parsing calls the appropriate > > functions to bind everything. > > > > Maybe the same for sample-api-script-added-before-access.html > > Yes, agreed that it makes sense to run the bindings tests in those > scenarios. Added to those files. Thanks for the explanations -- this LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chasej@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...
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 chasej@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iclelland@chromium.org Link to the patchset: https://codereview.chromium.org/2863593003/#ps60001 (title: "Rebase")
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": 60001, "attempt_start_ts": 1494889864598650,
"parent_rev": "28dd0bab32d0c43cba71995b99e2255d36e42b7e", "commit_rev":
"bf4fc3116e4ddc3bc718b95c571a87c143211f0a"}
Message was sent while issue was closed.
Description was changed from ========== A continuation from: https://codereview.chromium.org/2861673007/ This moves existing tests for enabling/disabling to be explicit bindings tests. When testing changes for bindings, many of the layout test files would fail, all for the same reason. Now, there is separation for token handling vs bindings. That is, testing that trials are enabled/disabled correctly, vs that IDL members are exposed correctly for a trial. As well, cleaned up the existing tests to use the helper methods introduced in the previous CL. Two separate CLs to make it easier to diff all changes. BUG=695123 ========== to ========== A continuation from: https://codereview.chromium.org/2861673007/ This moves existing tests for enabling/disabling to be explicit bindings tests. When testing changes for bindings, many of the layout test files would fail, all for the same reason. Now, there is separation for token handling vs bindings. That is, testing that trials are enabled/disabled correctly, vs that IDL members are exposed correctly for a trial. As well, cleaned up the existing tests to use the helper methods introduced in the previous CL. Two separate CLs to make it easier to diff all changes. BUG=695123 Review-Url: https://codereview.chromium.org/2863593003 Cr-Commit-Position: refs/heads/master@{#471944} Committed: https://chromium.googlesource.com/chromium/src/+/bf4fc3116e4ddc3bc718b95c571a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/bf4fc3116e4ddc3bc718b95c571a...
Message was sent while issue was closed.
Description was changed from ========== A continuation from: https://codereview.chromium.org/2861673007/ This moves existing tests for enabling/disabling to be explicit bindings tests. When testing changes for bindings, many of the layout test files would fail, all for the same reason. Now, there is separation for token handling vs bindings. That is, testing that trials are enabled/disabled correctly, vs that IDL members are exposed correctly for a trial. As well, cleaned up the existing tests to use the helper methods introduced in the previous CL. Two separate CLs to make it easier to diff all changes. BUG=695123 Review-Url: https://codereview.chromium.org/2863593003 Cr-Commit-Position: refs/heads/master@{#471944} Committed: https://chromium.googlesource.com/chromium/src/+/bf4fc3116e4ddc3bc718b95c571a... ========== to ========== Add Origin Trial layout tests for IDL bindings (part 2) A continuation from: https://codereview.chromium.org/2861673007/ This moves existing tests for enabling/disabling to be explicit bindings tests. When testing changes for bindings, many of the layout test files would fail, all for the same reason. Now, there is separation for token handling vs bindings. That is, testing that trials are enabled/disabled correctly, vs that IDL members are exposed correctly for a trial. As well, cleaned up the existing tests to use the helper methods introduced in the previous CL. Two separate CLs to make it easier to diff all changes. BUG=695123 Review-Url: https://codereview.chromium.org/2863593003 Cr-Commit-Position: refs/heads/master@{#471944} Committed: https://chromium.googlesource.com/chromium/src/+/bf4fc3116e4ddc3bc718b95c571a... ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
