Bug 192841 – [WPE][GTK] Purge use of g_assert() under TestWebKitAPI

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Michael Catanzaro]

The documentation of g_assert() says:

"""
The macro can be turned off in final releases of code by defining G_DISABLE_ASSERT when compiling the application, so code must not depend on any side effects from expr . Similarly, it must not be used in unit tests, otherwise the unit tests will be ineffective if compiled with G_DISABLE_ASSERT. Use g_assert_true() and related macros in unit tests instead.
"""

There are separate g_assert_*() macros for use in tests, which we use inconsistently currently. Use them always. This probably wasn't the best use of my time today, but it's been annoying me for a while and makes sense to do. We'll get nicer error messages now when tests fail.

The documentation also says not to use g_assert_not_reached() in tests, but there is no good replacement for that.

[Michael Catanzaro]

Note this will probably break some tests and testing locally is still not practical, so I'll be responsible for gardening API tests when this lands.

[Michael Catanzaro]

Ah, I thought I was done with this, but hit a problem and won't have a patch tonight:

../../Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitUserContentManager.cpp:44:61: error: invalid conversion from ‘WebKitUserContentManager*’ {aka ‘_WebKitUserContentManager*’} to ‘guint64’ {aka ‘long unsigned int’} [-fpermissive]
     g_assert_cmphex(webkit_web_view_get_user_content_manager(webView1.get()), ==, userContentManager1.get());
                                                             ^

The macro is defeated by C++ so we'll need to use g_assert_true() or write our own macro that does the casts for us.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #1)
> Note this will probably break some tests and testing locally is still not
> practical, so I'll be responsible for gardening API tests when this lands.

Actually all I have to do is compare before and after results. Before:

Unexpected failures (4)
    /WebKit2Gtk/TestAutomationSession
        /webkit/WebKitAutomationSession/request-session
    /WebKit2Gtk/TestWebKitAccessibility
        /webkit/WebKitAccessibility/atspi-basic-hierarchy
    /WTF/TestWTF
        WTF_DateMath.calculateLocalTimeOffset
        WTF.StringConcatenate_Unsigned

Unexpected timeouts (9)
    /WebKit2Gtk/TestInspector
        /webkit/WebKitWebInspector/custom-container-destroyed
        /webkit/WebKitWebInspector/manual-attach-detach
        /webkit/WebKitWebInspector/default
    /WebKit2Gtk/TestUIClient
        /webkit/WebKitWebView/geolocation-permission-requests
    /WebKit2Gtk/TestWebKitWebContext
        /webkit/WebKitWebContext/uri-scheme
    /WebKit2Gtk/TestWebExtensions
        /webkit/WebKitWebExtension/form-submission-steps
    /WTF/TestWTF
        WTF_WordLock.ManyContendedLongSections
        WTF_WordLock.ContendedShortSection
        WTF_Lock.ContendedShortSection

Unexpected passes (2)
    /WebKit2Gtk/TestWebViewEditor
        /webkit/WebKitWebView/editable/editable
    /WebKit2Gtk/TestUIClient
        /webkit/WebKitWebView/usermedia-enumeratedevices-permission-check


After:

Unexpected failures (11)
    /WebKit2Gtk/TestLoaderClient
        /webkit/WebKitURIResponse/http-headers
    /WebKit2Gtk/TestWebKitUserContentManager
        /webkit/WebKitUserContentManager/script-message-received
        /webkit/WebKitUserContentManager/script-message-in-world-received
    /WebKit2Gtk/TestSSL
        /webkit/WebKitWebView/tls-errors-policy
        /webkit/WebKitWebView/load-failed-with-tls-errors
        /webkit/WebKitWebView/tls-errors-redirect-to-http
        /webkit/WebKitWebView/tls-http-auth
    /WebKit2Gtk/TestAutomationSession
        /webkit/WebKitAutomationSession/request-session
    /WebKit2Gtk/TestWebKitAccessibility
        /webkit/WebKitAccessibility/atspi-basic-hierarchy
    /WTF/TestWTF
        WTF_DateMath.calculateLocalTimeOffset
        WTF.StringConcatenate_Unsigned

Unexpected timeouts (9)
    /WebKit2Gtk/TestInspector
        /webkit/WebKitWebInspector/custom-container-destroyed
        /webkit/WebKitWebInspector/manual-attach-detach
        /webkit/WebKitWebInspector/default
    /WebKit2Gtk/TestUIClient
        /webkit/WebKitWebView/geolocation-permission-requests
    /WebKit2Gtk/TestWebKitWebContext
        /webkit/WebKitWebContext/uri-scheme
    /WebKit2Gtk/TestWebExtensions
        /webkit/WebKitWebExtension/form-submission-steps
    /WTF/TestWTF
        WTF_WordLock.ManyContendedLongSections
        WTF_WordLock.ContendedShortSection
        WTF_Lock.ContendedShortSection

Unexpected passes (2)
    /WebKit2Gtk/TestWebViewEditor
        /webkit/WebKitWebView/editable/editable
    /WebKit2Gtk/TestUIClient
        /webkit/WebKitWebView/usermedia-enumeratedevices-permission-check


So looks like I maybe broke five tests. I'll upload the patch for early review without r?, need to examine those tests. Note I wound up redefining g_assert_cmphex() and g_assert_nonnull(), and then redefined g_assert_null() as well for good measure. Feels a bit icky but I think it's nicer than adding new macros; we can of course do that if desired.

[Michael Catanzaro]

Created attachment 357651 [details]
Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #3)
> So looks like I maybe broke five tests.

Seven... 11 - 4 = 7. I can count.

[Alicia Boya García]

(In reply to Michael Catanzaro from comment #0)
> There are separate g_assert_*() macros for use in tests, which we use
> inconsistently currently. Use them always. This probably wasn't the best use
> of my time today, but it's been annoying me for a while and makes sense to
> do. We'll get nicer error messages now when tests fail.

Why are these tests not using gtest assertion macros like multiplatform tests? It seems weird to me to use different assertion macros among platforms.

[Michael Catanzaro]

They're GLib tests using the GLib test framework. They don't use the gtest framework at all.

[Michael Catanzaro]

OK, I fixed the seven that I broke.

Of course, no guarantees for the tests that were already broken.

[Michael Catanzaro]

Created attachment 357744 [details]
Patch

[Michael Catanzaro]

Comment on attachment 357744 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357744&action=review

> Tools/TestWebKitAPI/glib/WebKitGLib/TestUtils.h:45
> +        auto __n1 = reinterpret_cast<intptr_t>(n1);                      \
> +        auto __n2 = reinterpret_cast<intptr_t>(n2);                      \
> +        if (__n1 cmp __n2) ;                                             \
> +        else {                                                           \
> +            g_assertion_message_cmpnum(G_LOG_DOMAIN, __FILE__, __LINE__, \
> +                G_STRFUNC, #n1 " " #cmp " " #n2, (long double)(__n1), #cmp, (long double)(__n2), 'x'); \

BTW this is definitely not portable (casting a pointer through an int never is... only the opposite is portable, and then we do a C-style cast to long double... and then to guint64 inside g_assertion_message_cmpnum). So I'm thinking we should probably implement our own assertion_message_cmpptr to use here.

[Michael Catanzaro]

Philip Withnall has suggested g_assert_true(p1 op p2) instead of g_assert_cmphex(p1, op, p2). The disadvantage is that won't print if one or the other is NULL.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #11)
> Philip Withnall has suggested g_assert_true(p1 op p2) instead of
> g_assert_cmphex(p1, op, p2). The disadvantage is that won't print if one or
> the other is NULL.

Switched to this, eliminating the need to redefine g_assert_cmphex().

Also, I can't reproduce the -Wpointer-arith warnings I was seeing recently. Maybe I had some broken g_assert_nonnull() in an earlier version of the patch. So we don't need to redefine g_assert_nonnull() anymore either. So we no longer need to add TestUtils.h or redefine any of the GLib macros at all. (I'll submit the NULL -> nullptr changes to GLib separately.) Much better.

Please review before this goes stale.

[Michael Catanzaro]

Created attachment 358207 [details]
Patch

[Michael Catanzaro]

Committed r239772: <https://trac.webkit.org/changeset/239772>

[Radar WebKit Bug Importer]

<rdar://problem/47146154>