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

Fix the error catching logic of should().throw() in audit.js #12606

Merged
merged 1 commit into from
Aug 31, 2018

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Aug 21, 2018

Context: #12606

Change the logic of should.throw() so we can handle 3 cases -

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

The generic error (except for DOMException) can be passed without
the second argument, but this change will enforce the second arg
when the expected error is a DOMException type.

This touches many test files, so the work will be done in several
steps:

  1. Change audit.js, audionodeoptions.js on both locations.
    (wpt, non-wpt)
  2. Update affected test files with the script.
  3. Update the rest of test files which can't be updated
    programmatically.

Bug: 865614
Test: All layout tests pass.
Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f
Reviewed-on: https://chromium-review.googlesource.com/1184146
Commit-Queue: Hongchan Choi [email protected]
Reviewed-by: Raymond Toy [email protected]
Cr-Commit-Position: refs/heads/master@{#587660}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already reviewed downstream.

@karlt
Copy link
Contributor

karlt commented Aug 22, 2018

@hoch, can you describe the exact situation, please, where the previous code
was causing a problem?

Is there a case where a test needs to accept either of a simple exception or a
DOMException? Wouldn't it be better to be specific about what is expected?

AIUI the intention of #11983 was
that tests would be specific about the kind of error that is thrown.

With this change, I don't see how a test could require that an exception is a
DOMException.

If the expected exception is a simple exception, then wouldn't it be better for
that test to require that the exception is a simple exception, by specifying
the constructor of the simple exception?

(It does look like the previous documentation "It can also be an instance of
either an Error or a DOMException" was incorrect, as the code required a
constructor and the DOMException constructor would not have a specific
name.)

@hoch
Copy link
Contributor

hoch commented Aug 22, 2018

should().throw() is an assertion to catch any kind of error. At least, that's why I made it.

Please consider this example:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/webaudio/audio-scheduled-source-basic.html

After the previous change (#11983), the test above started failing. If the assertion only expects to see DOMException, how can we catch things like TypeError or ReferenceError? rtoy@ and I thought generalize it to Error is okay, but feel free to let us know if you have something else in mind.

@karlt
Copy link
Contributor

karlt commented Aug 22, 2018

If the assertion only expects to see DOMException, how can we catch things like TypeError or ReferenceError?

The assertion would have only expected to see DOMException if expected was a string.
To catch simple exceptions like TypeError etc., should().throw(TypeError) was passed.

That catches bugs like https://bugzilla.mozilla.org/show_bug.cgi?id=1472095

/cc @padenot @bzbarsky

@hoch
Copy link
Contributor

hoch commented Aug 22, 2018

The previous change created ~10 test failures on our side. I don't have a strong opinion on this, but why do we want to care about DOMException specifically? IMO should().throw() should be generic enough to catch different types of errors or exceptions, and log its detail (type, name and etc) in the message.

@karlt
Copy link
Contributor

karlt commented Aug 22, 2018

The existing web platform tests were updated for the previous change. I would
have expected the only failures resulting from the changes to be in recent
tests that are yet to be upstreamed, unless audit.js from wpt is somehow being
used for tests outside wpt.

We care about DOMException specifically if the Web Audio API requires that a
DOMException is thrown.

If no parameters were passed to throw() then AFAIK it was still generic enough
to catch different types of exceptions.

If the Web Audio API requires that a simple exception such as TypeError is
thrown, then the framework was still generic enough to test also for that
specific exception.

Are you saying that there was a problem logging the detail of the failure?
Or do you not want to test the specific type of the exception?
Is there a reason why you only want to test the name and not the type?

@hoch
Copy link
Contributor

hoch commented Aug 23, 2018

We care about DOMException specifically if the Web Audio API requires that a
DOMException is thrown.

If no parameters were passed to throw() then AFAIK it was still generic enough
to catch different types of exceptions.

If we really want to specific about the type of error of exception, we should prevent the string argument in the assertion and enforce to use a specific type instead. Would it break many existing tests on your end? If not, I would like to remove the support on string type argument from throw() method.

I just did not have the context of this change, so this clarification really helped me understand. Please ignore what I said about "the detail of failure".

@hoch
Copy link
Contributor

hoch commented Aug 23, 2018

How about something like this?

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

Basically throw() will expect the name of exception with the first arg is DOMException.

@karlt
Copy link
Contributor

karlt commented Aug 23, 2018

That sounds fine to me, thank you. Mozilla is using audit.js with only
web-platform-tests and so there are no other tests that would need updating.

Just so you are aware, that would be different syntax from the
assert_throws() function which expects {object|number|string} for the
expected parameter. number or string mean to expect DOMExceptions.
object is for an argument like new TypeError().

* Assert an Exception with the expected code is thrown.

The current syntax is already somewhat different, and so this is not a big deal, but
this may be a good time to consider making the syntax more similar.

(The other thing I notice in assert_throws() is that it does not use
instanceof because that would fail when used on an exception from a different
window. To workaround that somewhat, names are limited to recognized names
and the code property is also tested. AFAIK web audio tests do not need to
care about different windows, and so I guess audit.js does not need to do
the same.

//We'd like to test that e instanceof the appropriate interface,
)

@hoch
Copy link
Contributor

hoch commented Aug 24, 2018

That is a good point. However, we actually created audit.js because we did not like much about what the test harness offers. So diverging from the harness is not a big deal from our perspective.

The latest patch I made (still working on WPT side) has this change and I think it's a bit clearer than we what had before.

@chromium-wpt-export-bot chromium-wpt-export-bot changed the title Update audit.js and fix error catching logic in should().throw() Fix the error catching logic of should().throw() in audit.js Aug 24, 2018
chromium-wpt-export-bot pushed a commit that referenced this pull request Aug 24, 2018
Context: #12606

Change the logic of should.throw() so we can handle 3 cases -

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

The generic error (except for DOMException) can be passed without
the second argument, but this change will enforce the second arg
when the expected error is a DOMException type.

This touches many test files, so the work will be done in several
steps:

1. Change audit.js on both locations (wpt, non-wpt)
2. Update affected test files with the script.
3. Update the rest of test files which can't be updated
  programmatically.

Bug: 865614
Test: All layout tests pass.
Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f
chromium-wpt-export-bot pushed a commit that referenced this pull request Aug 24, 2018
Context: #12606

Change the logic of should.throw() so we can handle 3 cases -

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

The generic error (except for DOMException) can be passed without
the second argument, but this change will enforce the second arg
when the expected error is a DOMException type.

This touches many test files, so the work will be done in several
steps:

1. Change audit.js, audionodeoptions.js on both locations.
  (wpt, non-wpt)
2. Update affected test files with the script.
3. Update the rest of test files which can't be updated
  programmatically.

Bug: 865614
Test: All layout tests pass.
Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f
chromium-wpt-export-bot pushed a commit that referenced this pull request Aug 24, 2018
Context: #12606

Change the logic of should.throw() so we can handle 3 cases -

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

The generic error (except for DOMException) can be passed without
the second argument, but this change will enforce the second arg
when the expected error is a DOMException type.

This touches many test files, so the work will be done in several
steps:

1. Change audit.js, audionodeoptions.js on both locations.
  (wpt, non-wpt)
2. Update affected test files with the script.
3. Update the rest of test files which can't be updated
  programmatically.

Bug: 865614
Test: All layout tests pass.
Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f
chromium-wpt-export-bot pushed a commit that referenced this pull request Aug 24, 2018
Context: #12606

Change the logic of should.throw() so we can handle 3 cases -

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

The generic error (except for DOMException) can be passed without
the second argument, but this change will enforce the second arg
when the expected error is a DOMException type.

This touches many test files, so the work will be done in several
steps:

1. Change audit.js, audionodeoptions.js on both locations.
  (wpt, non-wpt)
2. Update affected test files with the script.
3. Update the rest of test files which can't be updated
  programmatically.

Bug: 865614
Test: All layout tests pass.
Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f
chromium-wpt-export-bot pushed a commit that referenced this pull request Aug 30, 2018
Context: #12606

Change the logic of should.throw() so we can handle 3 cases -

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

The generic error (except for DOMException) can be passed without
the second argument, but this change will enforce the second arg
when the expected error is a DOMException type.

This touches many test files, so the work will be done in several
steps:

1. Change audit.js, audionodeoptions.js on both locations.
  (wpt, non-wpt)
2. Update affected test files with the script.
3. Update the rest of test files which can't be updated
  programmatically.

Bug: 865614
Test: All layout tests pass.
Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 30, 2018
Context: web-platform-tests/wpt#12606

Change the logic of should.throw() so we can handle 3 cases -

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

The generic error (except for DOMException) can be passed without
the second argument, but this change will enforce the second arg
when the expected error is a DOMException type.

This touches many test files, so the work will be done in several
steps:

1. Change audit.js, audionodeoptions.js on both locations.
  (wpt, non-wpt)
2. Update affected test files with the script.
3. Update the rest of test files which can't be updated
  programmatically.

Bug: 865614
Test: All layout tests pass.
Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f
Reviewed-on: https://chromium-review.googlesource.com/1184146
Commit-Queue: Hongchan Choi <[email protected]>
Reviewed-by: Raymond Toy <[email protected]>
Cr-Commit-Position: refs/heads/master@{#587660}
Context: #12606

Change the logic of should.throw() so we can handle 3 cases -

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

The generic error (except for DOMException) can be passed without
the second argument, but this change will enforce the second arg
when the expected error is a DOMException type.

This touches many test files, so the work will be done in several
steps:

1. Change audit.js, audionodeoptions.js on both locations.
  (wpt, non-wpt)
2. Update affected test files with the script.
3. Update the rest of test files which can't be updated
  programmatically.

Bug: 865614
Test: All layout tests pass.
Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f
Reviewed-on: https://chromium-review.googlesource.com/1184146
Commit-Queue: Hongchan Choi <[email protected]>
Reviewed-by: Raymond Toy <[email protected]>
Cr-Commit-Position: refs/heads/master@{#587660}
@foolip
Copy link
Member

foolip commented Aug 31, 2018

This PR has not been merged automatically because the Firefox job is failing due to taking too long. (Travis timeout is 50 minutes.)

Suspecting that might be because the tests were made broken (timeout) I tested some of the affected tests before and after the changes but couldn't observe any difference. So this is just #7660 again, I think. I'll force merge.

This is a case where #7475 would have come in handy.

@foolip foolip merged commit 4fd41de into master Aug 31, 2018
@foolip foolip deleted the chromium-export-cl-1184146 branch August 31, 2018 14:13
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 5, 2018
….throw() in audit.js, a=testonly

Automatic update from web-platform-testsFix the error catching logic of should().throw() in audit.js

Context: web-platform-tests/wpt#12606

Change the logic of should.throw() so we can handle 3 cases -

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

The generic error (except for DOMException) can be passed without
the second argument, but this change will enforce the second arg
when the expected error is a DOMException type.

This touches many test files, so the work will be done in several
steps:

1. Change audit.js, audionodeoptions.js on both locations.
  (wpt, non-wpt)
2. Update affected test files with the script.
3. Update the rest of test files which can't be updated
  programmatically.

Bug: 865614
Test: All layout tests pass.
Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f
Reviewed-on: https://chromium-review.googlesource.com/1184146
Commit-Queue: Hongchan Choi <[email protected]>
Reviewed-by: Raymond Toy <[email protected]>
Cr-Commit-Position: refs/heads/master@{#587660}

--

wpt-commits: 4fd41de6ecf73b21534c81fc0f516defdca50ab1
wpt-pr: 12606
jankeromnes pushed a commit to jankeromnes/gecko that referenced this pull request Sep 5, 2018
….throw() in audit.js, a=testonly

Automatic update from web-platform-testsFix the error catching logic of should().throw() in audit.js

Context: web-platform-tests/wpt#12606

Change the logic of should.throw() so we can handle 3 cases -

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

The generic error (except for DOMException) can be passed without
the second argument, but this change will enforce the second arg
when the expected error is a DOMException type.

This touches many test files, so the work will be done in several
steps:

1. Change audit.js, audionodeoptions.js on both locations.
  (wpt, non-wpt)
2. Update affected test files with the script.
3. Update the rest of test files which can't be updated
  programmatically.

Bug: 865614
Test: All layout tests pass.
Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f
Reviewed-on: https://chromium-review.googlesource.com/1184146
Commit-Queue: Hongchan Choi <[email protected]>
Reviewed-by: Raymond Toy <[email protected]>
Cr-Commit-Position: refs/heads/master@{#587660}

--

wpt-commits: 4fd41de6ecf73b21534c81fc0f516defdca50ab1
wpt-pr: 12606
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
….throw() in audit.js, a=testonly

Automatic update from web-platform-testsFix the error catching logic of should().throw() in audit.js

Context: web-platform-tests/wpt#12606

Change the logic of should.throw() so we can handle 3 cases -

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

The generic error (except for DOMException) can be passed without
the second argument, but this change will enforce the second arg
when the expected error is a DOMException type.

This touches many test files, so the work will be done in several
steps:

1. Change audit.js, audionodeoptions.js on both locations.
  (wpt, non-wpt)
2. Update affected test files with the script.
3. Update the rest of test files which can't be updated
  programmatically.

Bug: 865614
Test: All layout tests pass.
Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f
Reviewed-on: https://chromium-review.googlesource.com/1184146
Commit-Queue: Hongchan Choi <hongchanchromium.org>
Reviewed-by: Raymond Toy <rtoychromium.org>
Cr-Commit-Position: refs/heads/master{#587660}

--

wpt-commits: 4fd41de6ecf73b21534c81fc0f516defdca50ab1
wpt-pr: 12606

UltraBlame original commit: 650ffc22e4fef5b4d464b33a09663b47ceb620c5
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
….throw() in audit.js, a=testonly

Automatic update from web-platform-testsFix the error catching logic of should().throw() in audit.js

Context: web-platform-tests/wpt#12606

Change the logic of should.throw() so we can handle 3 cases -

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

The generic error (except for DOMException) can be passed without
the second argument, but this change will enforce the second arg
when the expected error is a DOMException type.

This touches many test files, so the work will be done in several
steps:

1. Change audit.js, audionodeoptions.js on both locations.
  (wpt, non-wpt)
2. Update affected test files with the script.
3. Update the rest of test files which can't be updated
  programmatically.

Bug: 865614
Test: All layout tests pass.
Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f
Reviewed-on: https://chromium-review.googlesource.com/1184146
Commit-Queue: Hongchan Choi <hongchanchromium.org>
Reviewed-by: Raymond Toy <rtoychromium.org>
Cr-Commit-Position: refs/heads/master{#587660}

--

wpt-commits: 4fd41de6ecf73b21534c81fc0f516defdca50ab1
wpt-pr: 12606

UltraBlame original commit: 650ffc22e4fef5b4d464b33a09663b47ceb620c5
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
….throw() in audit.js, a=testonly

Automatic update from web-platform-testsFix the error catching logic of should().throw() in audit.js

Context: web-platform-tests/wpt#12606

Change the logic of should.throw() so we can handle 3 cases -

should(someExpression).throw();
should(someExpression).throw(TypeError);
should(someExpression).throw(DOMException, 'NotSupportedError');

The generic error (except for DOMException) can be passed without
the second argument, but this change will enforce the second arg
when the expected error is a DOMException type.

This touches many test files, so the work will be done in several
steps:

1. Change audit.js, audionodeoptions.js on both locations.
  (wpt, non-wpt)
2. Update affected test files with the script.
3. Update the rest of test files which can't be updated
  programmatically.

Bug: 865614
Test: All layout tests pass.
Change-Id: I16acacb26c194a0ff950aca05e931195bf55167f
Reviewed-on: https://chromium-review.googlesource.com/1184146
Commit-Queue: Hongchan Choi <hongchanchromium.org>
Reviewed-by: Raymond Toy <rtoychromium.org>
Cr-Commit-Position: refs/heads/master{#587660}

--

wpt-commits: 4fd41de6ecf73b21534c81fc0f516defdca50ab1
wpt-pr: 12606

UltraBlame original commit: 650ffc22e4fef5b4d464b33a09663b47ceb620c5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants