Re: [PATCH 1/3] jscript: test that ActiveScriptSite_OnScriptError is called on script failure. (resend)

[Reece Dunn]

On 7 October 2010 14:35, Jacek Caban <jacek(a)codeweavers.com> wrote:
>  Hi Reece,
>
> These tests need more thoughts. They should show how jscript exceptions are
> translated into IActiveScriptError. Your code doesn't provide mechanism to
> do so, so I would suggest to wait with sending patches that don't move us
> any forward until you have a clue how it should work. Otherwise you will
> have to deal with eventually bad decisions for no good reason.

Ok, so if I keep the current tests/ActiveScriptSite as they are and
have something like:

   test_script_error(source, scode, description, lineno, charpos, linesource) {
      EXPECT CALL ActiveScriptSite_OnScriptError
      hres = parse_script_with_error(source);
      ok(hres == 0x8012345, ...)

      CHECK CALL ActiveScriptSite_OnScriptError
      ok(script_excep.scode == scode, ...)
      ok(script_excep.bstrDescription == description, ...)
      ok(script_lineno == lineno, ...)
      ok(script_charpos == charpos, ...)
      ok(script_linesource == linesource, ...)
   }

   test_script_errors() {
      test_script_error("var foo = new Object();\nfoo.bar;",
0x8012345, "Object does not support this property", 20, 11,
"foo.bar;")
      test_script_error("new <>%^^;", 0x8012322, "Syntax error", 0, 0,
"new <>%^^;")
      ...
   }

where parse_script_with_error does the same thing as parse_script, but
uses an ActiveScriptSite implementation that checks OnScriptError
calls and stores the IActiveScriptError object for testing within
test_script_error (like with my 3rd patch, but checking specific
values).

   1/ Does this look like a sensible approach to you?

   2/ Do we care about the tests working on non-English locales (not
sure how to get the above working on other languages -- specifically
the description text)?

   3/ Are we ok with making wine match the error description strings
exactly, given the above tests?

NOTE: 2 and 3 can be avoided by just checking the scode and checking
that the description is set and non-NULL (as per my 3rd patch).
> Also merging parsing_htmlscript is not a good idea. Its purpose is different
> than parse_script.

Ok, I'll keep them separate.

- Reece

[PATCH 1/3] jscript: test that ActiveScriptSite_OnScriptError is called on script failure. (resend)

[Jacek Caban]

On 10/7/10 4:22 PM, Reece Dunn wrote:
> On 7 October 2010 14:35, Jacek Caban<jacek(a)codeweavers.com>  wrote:
> > Hi Reece,
> >
> > These tests need more thoughts. They should show how jscript exceptions are
> > translated into IActiveScriptError. Your code doesn't provide mechanism to
> > do so, so I would suggest to wait with sending patches that don't move us
> > any forward until you have a clue how it should work. Otherwise you will
> > have to deal with eventually bad decisions for no good reason.
> > Ok, so if I keep the current tests/ActiveScriptSite as they are and
> > have something like:
>
> test_script_error(source, scode, description, lineno, charpos, linesource) {
>        EXPECT CALL ActiveScriptSite_OnScriptError
>        hres = parse_script_with_error(source);
>        ok(hres == 0x8012345, ...)
>
> CHECK CALL ActiveScriptSite_OnScriptError
>        ok(script_excep.scode == scode, ...)
>        ok(script_excep.bstrDescription == description, ...)
>        ok(script_lineno == lineno, ...)
>        ok(script_charpos == charpos, ...)
>        ok(script_linesource == linesource, ...)
>     }
>
> test_script_errors() {
>        test_script_error("var foo = new Object();\nfoo.bar;",
> 0x8012345, "Object does not support this property", 20, 11,
> "foo.bar;")
>        test_script_error("new<>%^^;", 0x8012322, "Syntax error", 0, 0,
> "new<>%^^;")
>        ...
>     }
>
> where parse_script_with_error does the same thing as parse_script, but
> uses an ActiveScriptSite implementation that checks OnScriptError
> calls and stores the IActiveScriptError object for testing within
> test_script_error (like with my 3rd patch, but checking specific
> values).
>
> 1/ Does this look like a sensible approach to you?

Yes, this idea sounds good.
> 2/ Do we care about the tests working on non-English locales (not
> sure how to get the above working on other languages -- specifically
> the description text)?

Yes, we care. The easiest way to handle it is checking the locale and 
skipping tests on non-English locales.
> 3/ Are we ok with making wine match the error description strings
> exactly, given the above tests?

We don't really care if strings are exactly the same. It would be nice, 
however, to test them and the only sane way I can see to achieve it is by 
making them exactly the same as native and comparing to expected value. I'd 
expect these values to be the same as in jscript *Error objects, so I'd 
prefer to extend api.js to check strings. In C part we could make this test 
optional and use it in very few places just to make sure it matches 
jscript-visible values.

Jacek