Re: [PATCH] vbscript/tests: Show IActiveScriptSite OnScriptError is called on error

[Jacek Caban]

Hi Alistair,

On 10/26/15 09:56, Alistair Leslie-Hughes wrote:
> Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>
> ---
>  dlls/vbscript/tests/run.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/vbscript/tests/run.c b/dlls/vbscript/tests/run.c
> index a86711a..e625136 100644
> --- a/dlls/vbscript/tests/run.c
> +++ b/dlls/vbscript/tests/run.c
> @@ -100,6 +100,7 @@ DEFINE_EXPECT(Next);
>  DEFINE_EXPECT(GetWindow);
>  DEFINE_EXPECT(GetUIBehavior);
>  DEFINE_EXPECT(EnableModeless);
> +DEFINE_EXPECT(on_script_error);

Please use OnScriptError for consistency.
> #define DISPID_GLOBAL_REPORTSUCCESS 1000
>  #define DISPID_GLOBAL_TRACE         1001
> @@ -138,6 +139,7 @@ static const char *test_name = "(null)";
>  static int test_counter;
>  static SCRIPTUICHANDLING uic_handling = SCRIPTUICHANDLING_NOUIERROR;
>  static IDispatchEx testObj;
> +static BOOL overwrite_return = FALSE;

How about using HRESULT here...
> static BSTR a2bstr(const char *str)
>  {
> @@ -1623,7 +1625,22 @@ static HRESULT WINAPI ActiveScriptSite_OnStateChange(IActiveScriptSite *iface, S
>
> static HRESULT WINAPI ActiveScriptSite_OnScriptError(IActiveScriptSite *iface, IActiveScriptError *pscripterror)
>  {
> -    return E_NOTIMPL;
> +    HRESULT hr;
> +    EXCEPINFO ei = {0};
> +
> +    CHECK_EXPECT(on_script_error);
> +
> +    hr = IActiveScriptError_GetExceptionInfo(pscripterror, &ei);
> +    ok(hr == S_OK, "GetExceptionInfo %x\n", hr);
> +
> +    if(overwrite_return)
> +    {
> +        overwrite_return = FALSE;
> +        return S_OK;
> +    }
> +
> +    /* We return the EXCEPINFO error since returning S_OK means we have handled the error */
> +    return ei.scode;

... and return it here for simplicity?

Thanks,
Jacek

[PATCH] vbscript/tests: Show IActiveScriptSite OnScriptError is called on error

[Alistair Leslie-Hughes]

Hi Jacek,

On 26/10/15 22:23, Jacek Caban wrote:
> Please use OnScriptError for consistency.
> Sure.
> How about using HRESULT here...
> Not sure how this would work without changing every test that throws an 
> exception. I was trying to show that if S_OK is returned from 
> OnScriptError then SCRIPT_E_REPORTED is the expected return.

Best Regards
  Alistair Leslie-Hughes

[PATCH] vbscript/tests: Show IActiveScriptSite OnScriptError is called on error

[Alistair Leslie-Hughes]

Hi Jacek,

On 26/10/15 22:23, Jacek Caban wrote:
> Please use OnScriptError for consistency.
> Sure.
> How about using HRESULT here...
> Not sure how this would work without changing every test that throws an 
> exception. I was trying to show that if S_OK is returned from 
> OnScriptError then SCRIPT_E_REPORTED is the expected return.

Best Regards
  Alistair Leslie-Hughes

[PATCH] vbscript/tests: Show IActiveScriptSite OnScriptError is called on error

[Jacek Caban]

Hi Alistair,

Sorry for late response.

On 10/27/15 09:56, Alistair Leslie-Hughes wrote:
> > How about using HRESULT here...
> > Not sure how this would work without changing every test that throws
> > an exception. I was trying to show that if S_OK is returned from
> > OnScriptError then SCRIPT_E_REPORTED is the expected return.

I'm not sure why it wouldn't work. We currently return E_NOTIMPL and
it's fine. In your case, you could add onerror_hres, set it to E_NOTIMPL
and return it from OnScriptError. To test what you meant to test, you'd
change its value temporary to S_OK.

The reason I'd prefer it is that right now you test two things at the
time in a way that will complicate future development. When we implement
OnScriptError callback, we will hit GetExceptionInfo failure, which we
will have to work around. Obviously implementing GetExceptionInfo first
would be even better, but for that we'd need more tests, those wouldn't
be enough anyway.

Thanks,
Jacek