Bug 192841 - [WPE][GTK] Purge use of g_assert() under TestWebKitAPI
Summary: [WPE][GTK] Purge use of g_assert() under TestWebKitAPI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-18 19:22 PST by Michael Catanzaro
Modified: 2019-01-09 07:32 PST (History)
6 users (show)

See Also:


Attachments
Patch (466.24 KB, patch)
2018-12-18 21:28 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (466.24 KB, patch)
2018-12-19 15:56 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (460.16 KB, patch)
2019-01-02 14:35 PST, Michael Catanzaro
cgarcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2018-12-18 19:22:16 PST
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.
Comment 1 Michael Catanzaro 2018-12-18 19:30:35 PST
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.
Comment 2 Michael Catanzaro 2018-12-18 19:47:51 PST
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.
Comment 3 Michael Catanzaro 2018-12-18 21:23:48 PST
(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.
Comment 4 Michael Catanzaro 2018-12-18 21:28:20 PST
Created attachment 357651 [details]
Patch
Comment 5 Michael Catanzaro 2018-12-18 21:29:01 PST
(In reply to Michael Catanzaro from comment #3)
> So looks like I maybe broke five tests.

Seven... 11 - 4 = 7. I can count.
Comment 6 Alicia Boya García 2018-12-19 08:06:07 PST
(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.
Comment 7 Michael Catanzaro 2018-12-19 09:32:49 PST
They're GLib tests using the GLib test framework. They don't use the gtest framework at all.
Comment 8 Michael Catanzaro 2018-12-19 15:56:15 PST
OK, I fixed the seven that I broke.

Of course, no guarantees for the tests that were already broken.
Comment 9 Michael Catanzaro 2018-12-19 15:56:43 PST
Created attachment 357744 [details]
Patch
Comment 10 Michael Catanzaro 2018-12-19 18:15:33 PST
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.
Comment 11 Michael Catanzaro 2018-12-20 08:31:39 PST
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.
Comment 12 Michael Catanzaro 2019-01-02 14:34:14 PST
(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.
Comment 13 Michael Catanzaro 2019-01-02 14:35:15 PST
Created attachment 358207 [details]
Patch
Comment 14 Michael Catanzaro 2019-01-09 07:31:04 PST
Committed r239772: <https://trac.webkit.org/changeset/239772>
Comment 15 Radar WebKit Bug Importer 2019-01-09 07:32:30 PST
<rdar://problem/47146154>