[PATCH 1/4] wininet/tests: Update InternetErrorDlg tests taking recent Windows 10 results as a base.

[Dmitry Timoshkov]

Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru>
---
 dlls/wininet/tests/internet.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/dlls/wininet/tests/internet.c b/dlls/wininet/tests/internet.c
index ef1adefd1a3..c61a9793a95 100644
--- a/dlls/wininet/tests/internet.c
+++ b/dlls/wininet/tests/internet.c
@@ -1483,20 +1483,21 @@ static void test_InternetErrorDlg(void)
         { ERROR_INTERNET_SEC_CERT_CN_INVALID    , ERROR_CANCELLED, 0 },
         { ERROR_INTERNET_HTTP_TO_HTTPS_ON_REDIR , ERROR_SUCCESS, 0 },
         { ERROR_INTERNET_HTTPS_TO_HTTP_ON_REDIR , ERROR_SUCCESS, FLAG_TODO },
-        { ERROR_INTERNET_MIXED_SECURITY         , ERROR_CANCELLED, FLAG_TODO },
-        { ERROR_INTERNET_CHG_POST_IS_NON_SECURE , ERROR_CANCELLED, FLAG_TODO },
+        { ERROR_INTERNET_MIXED_SECURITY         , ERROR_CANCELLED },
+        { ERROR_INTERNET_CHG_POST_IS_NON_SECURE , ERROR_SUCCESS, FLAG_TODO },
         { ERROR_INTERNET_POST_IS_NON_SECURE     , ERROR_SUCCESS, 0 },
-        { ERROR_INTERNET_CLIENT_AUTH_CERT_NEEDED, ERROR_CANCELLED, FLAG_NEEDREQ|FLAG_TODO },
+        { ERROR_INTERNET_CLIENT_AUTH_CERT_NEEDED, ERROR_CANCELLED, FLAG_NEEDREQ },
         { ERROR_INTERNET_INVALID_CA             , ERROR_CANCELLED, 0 },
-        { ERROR_INTERNET_HTTPS_HTTP_SUBMIT_REDIR, ERROR_CANCELLED, FLAG_TODO },
-        { ERROR_INTERNET_INSERT_CDROM           , ERROR_CANCELLED, FLAG_TODO|FLAG_NEEDREQ|FLAG_UNIMPL },
+        { ERROR_INTERNET_HTTPS_HTTP_SUBMIT_REDIR, ERROR_CANCELLED },
+        { ERROR_INTERNET_INSERT_CDROM           , ERROR_CANCELLED, FLAG_NEEDREQ|FLAG_UNIMPL },
         { ERROR_INTERNET_SEC_CERT_ERRORS        , ERROR_CANCELLED, 0 },
         { ERROR_INTERNET_SEC_CERT_REV_FAILED    , ERROR_CANCELLED, 0 },
         { ERROR_HTTP_COOKIE_NEEDS_CONFIRMATION  , ERROR_HTTP_COOKIE_DECLINED, FLAG_TODO },
-        { ERROR_INTERNET_BAD_AUTO_PROXY_SCRIPT  , ERROR_CANCELLED, FLAG_TODO },
-        { ERROR_INTERNET_UNABLE_TO_DOWNLOAD_SCRIPT, ERROR_CANCELLED, FLAG_TODO },
-        { ERROR_HTTP_REDIRECT_NEEDS_CONFIRMATION, ERROR_CANCELLED, FLAG_TODO },
+        { ERROR_INTERNET_BAD_AUTO_PROXY_SCRIPT  , ERROR_CANCELLED },
+        { ERROR_INTERNET_UNABLE_TO_DOWNLOAD_SCRIPT, ERROR_CANCELLED },
+        { ERROR_HTTP_REDIRECT_NEEDS_CONFIRMATION, ERROR_CANCELLED },
         { ERROR_INTERNET_SEC_CERT_REVOKED       , ERROR_CANCELLED, 0 },
+        { ERROR_INTERNET_SEC_CERT_WEAK_SIGNATURE, ERROR_CANCELLED, 0 },
     };
 
     res = InternetErrorDlg(NULL, NULL, ERROR_INTERNET_SEC_CERT_ERRORS, 0, NULL);
@@ -1560,14 +1561,16 @@ static void test_InternetErrorDlg(void)
                 expected = NTE_PROV_TYPE_NOT_DEF;
             break;
         case ERROR_INTERNET_CHG_POST_IS_NON_SECURE:
-            if(res == ERROR_SUCCESS) /* win10 returns ERROR_SUCCESS */
-                expected = ERROR_SUCCESS;
+            if(res == ERROR_CANCELLED) /* before win10 returns ERROR_CANCELLED */
+                expected = ERROR_CANCELLED;
             break;
         default: break;
         }
 
-        if (expected == ERROR_NOT_SUPPORTED && res == ERROR_CANCELLED) /* Win10 1607+ */
+        if(expected == ERROR_NOT_SUPPORTED && res == ERROR_CANCELLED) /* Win10 1607+ */
             expected = ERROR_CANCELLED;
+        else if(expected == ERROR_CANCELLED && res == ERROR_NOT_SUPPORTED) /* XP, Win7, Win8 */
+            expected = ERROR_NOT_SUPPORTED;
 
         todo_wine_if(test_flags & FLAG_TODO)
             ok(res == expected, "Got %d, expected %d (%d)\n", res, expected, i);
@@ -1583,7 +1586,7 @@ static void test_InternetErrorDlg(void)
             expected = ERROR_INVALID_PARAMETER;
 
         res = InternetErrorDlg(hwnd, NULL, i, FLAGS_ERROR_UI_FLAGS_NO_UI, NULL);
-        todo_wine_if( test_flags & FLAG_TODO || i == ERROR_INTERNET_INCORRECT_PASSWORD)
+        todo_wine_if ((test_flags & FLAG_TODO) || i == ERROR_INTERNET_INCORRECT_PASSWORD || i == ERROR_INTERNET_INSERT_CDROM || i == ERROR_INTERNET_CLIENT_AUTH_CERT_NEEDED)
             ok(res == expected, "Got %d, expected %d (%d)\n", res, expected, i);
     }
 
-- 
2.31.1

[Dmitry Timoshkov]

Can I get a review of these patches please?

-- 
Dmitry.

[Jacek Caban]

Hi Dmitry,

On 8/6/21 2:43 PM, Dmitry Timoshkov wrote:
> @@ -1583,7 +1586,7 @@ static void test_InternetErrorDlg(void)
>               expected = ERROR_INVALID_PARAMETER;
>
> res = InternetErrorDlg(hwnd, NULL, i, FLAGS_ERROR_UI_FLAGS_NO_UI, NULL);
> -        todo_wine_if( test_flags & FLAG_TODO || i == ERROR_INTERNET_INCORRECT_PASSWORD)
> +        todo_wine_if ((test_flags & FLAG_TODO) || i == ERROR_INTERNET_INCORRECT_PASSWORD || i == ERROR_INTERNET_INSERT_CDROM || i == ERROR_INTERNET_CLIENT_AUTH_CERT_NEEDED)

Is there a reason why you don't use FLAG_TODO for those? It may not 
match what you need for your new tests, but why are you changing half of 
its usage here?


Thanks,

Jacek

[Dmitry Timoshkov]

Hi Jacek,

Thanks for the review.

Jacek Caban <jacek(a)codeweavers.com> wrote:
> > @@ -1583,7 +1586,7 @@ static void test_InternetErrorDlg(void)
> >               expected = ERROR_INVALID_PARAMETER;
> >
> > res = InternetErrorDlg(hwnd, NULL, i, FLAGS_ERROR_UI_FLAGS_NO_UI, NULL);
> > -        todo_wine_if( test_flags & FLAG_TODO || i == ERROR_INTERNET_INCORRECT_PASSWORD)
> > +        todo_wine_if ((test_flags & FLAG_TODO) || i == ERROR_INTERNET_INCORRECT_PASSWORD || i == ERROR_INTERNET_INSERT_CDROM || i == ERROR_INTERNET_CLIENT_AUTH_CERT_NEEDED)
>
> Is there a reason why you don't use FLAG_TODO for those? It may not 
> match what you need for your new tests, but why are you changing half of 
> its usage here?

Because using FLAG_TODO is fragile due to inconsistent behaviour, and it
fails in new or old tests. I tried different approaches in hope to make
it more clean and less controvercial regarding FLAG_TODO usage, but finally
decided to have it this way. I'm still open to suggestions though, however
I haven't found anything better while changing the tests to follow recent
Windows behaviour.

-- 
Dmitry.

[Jacek Caban]

On 8/26/21 9:33 PM, Dmitry Timoshkov wrote:
> Hi Jacek,
>
> Thanks for the review.
>
> Jacek Caban <jacek(a)codeweavers.com> wrote:
> > > @@ -1583,7 +1586,7 @@ static void test_InternetErrorDlg(void)
> > >                expected = ERROR_INVALID_PARAMETER;
> > >
> > > res = InternetErrorDlg(hwnd, NULL, i, FLAGS_ERROR_UI_FLAGS_NO_UI, NULL);
> > > -        todo_wine_if( test_flags & FLAG_TODO || i == ERROR_INTERNET_INCORRECT_PASSWORD)
> > > +        todo_wine_if ((test_flags & FLAG_TODO) || i == ERROR_INTERNET_INCORRECT_PASSWORD || i == ERROR_INTERNET_INSERT_CDROM || i == ERROR_INTERNET_CLIENT_AUTH_CERT_NEEDED)
> >
> > Is there a reason why you don't use FLAG_TODO for those? It may not
> > match what you need for your new tests, but why are you changing half of
> > its usage here?
> > Because using FLAG_TODO is fragile due to inconsistent behaviour, and it
> > fails in new or old tests. I tried different approaches in hope to make
> > it more clean and less controvercial regarding FLAG_TODO usage, but finally
> > decided to have it this way. I'm still open to suggestions though, however
> > I haven't found anything better while changing the tests to follow recent
> > Windows behaviour.

If you remove most of them, then you may remove all of them as well. If 
you remove most of them, you may as well replace the existing mechanism. 
How about something like this:

https://testbot.winehq.org/JobDetails.pl?Key=96801


Thanks,

Jacek

[Dmitry Timoshkov]

Jacek Caban <jacek(a)codeweavers.com> wrote:
> > > > @@ -1583,7 +1586,7 @@ static void test_InternetErrorDlg(void)
> > > >                expected = ERROR_INVALID_PARAMETER;
> > > >
> > > > res = InternetErrorDlg(hwnd, NULL, i, FLAGS_ERROR_UI_FLAGS_NO_UI, NULL);
> > > > -        todo_wine_if( test_flags & FLAG_TODO || i == ERROR_INTERNET_INCORRECT_PASSWORD)
> > > > +        todo_wine_if ((test_flags & FLAG_TODO) || i == ERROR_INTERNET_INCORRECT_PASSWORD || i == ERROR_INTERNET_INSERT_CDROM || i == ERROR_INTERNET_CLIENT_AUTH_CERT_NEEDED)
> > >
> > > Is there a reason why you don't use FLAG_TODO for those? It may not
> > > match what you need for your new tests, but why are you changing half of
> > > its usage here?
> > > Because using FLAG_TODO is fragile due to inconsistent behaviour, and it
> > > fails in new or old tests. I tried different approaches in hope to make
> > > it more clean and less controvercial regarding FLAG_TODO usage, but finally
> > > decided to have it this way. I'm still open to suggestions though, however
> > > I haven't found anything better while changing the tests to follow recent
> > > Windows behaviour.
>
> If you remove most of them, then you may remove all of them as well. If 
> you remove most of them, you may as well replace the existing mechanism. 
> How about something like this:
>
> https://testbot.winehq.org/JobDetails.pl?Key=96801

Thanks for keeping this going, the test and the results look good to me,
though I'd remove the use of broken() inside of an if statement. Are you
planning to submit this patch?

-- 
Dmitry.