[Webkit-unassigned] [Bug 188696] beforeunload interoperability issues with a throwing return

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=188696

--- Comment #18 from Ryosuke Niwa <rniwa at webkit.org> ---
(In reply to PhistucK from comment #17)
>
> > Comment on attachment 347442 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=347442&action=review
> > 
> > Thanks for contributing this patch. Seems like a good change to make! Almost
> > there, but needs a little work. Need to add a test, and there’s a mistake in
> > the patch that a good test would find.
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        No new tests .
> > 
> > Normally we’d say *why* there are no new tests.
> 
> I was too embarrassed to write, "Because, hell, I did not even test it
> manually". :P

We definitely need a layout test for this code change.

> > Ideally another test will cover the case where the valueOf
> > function does not return an exception and ensure that it is called exactly
> > once.
> 
> Huh, would you have added/thought about adding that test if I did not write
> that mistake?

The fact our existing test coverage didn't catch the bug Darin pointed out is a good evidence that we need more tests in this area. In general, it's a good practice to add more test coverage whenever someone makes a coding mistake, and that coding mistake wasn't caught by an existing test case.

This is why, for example, when I fix a bug or implement a new feature, I'd purposefully not implement the full fix, and see if any existing tests or imported tests would fail. Ideally, we would be testing every line of code we write one way or another. In some cases that's not practical due to timing etc... but we should strive to do so as much as we can.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180820/9ef9aaf2/attachment-0001.html>