Created attachment 206961 [details] [review]
tests: add ephy-web-view test

Currently only testing ephy_web_view_load_url

Review of attachment 206961 [details] [review]:

::: tests/ephy-web-view.c
@@ +73,3 @@
+  GMainLoop *loop;
+  EphyWebView *view;
+} Fixture;

Don't really need the loop now, perhaps will later. Should I take it out?

@@ +92,3 @@
+{
+  g_assert (EPHY_IS_WEB_VIEW (fixture->view));
+}

I wonder if tests should include this kind of cases. Isn't it too superfluous?

Review of attachment 206961 [details] [review]:

::: tests/ephy-web-view.c
@@ +33,3 @@
+#include <glib/gstdio.h>
+#include <gtk/gtk.h>
+#include <string.h>

Used?

@@ +92,3 @@
+{
+  g_assert (EPHY_IS_WEB_VIEW (fixture->view));
+}

It's implictly tested by the others, so I guess it's a bit silly yes.

@@ +96,3 @@
+typedef struct {
+  const char *url;
+  const char *loaded_url;

I insist about this thing freaking me out!

@@ +106,3 @@
+  { "gnome.org:80", "http://gnome.org:80/" },
+  { "gnome", "http://gnome" },
+#endif

Just kill this for now.

@@ +123,3 @@
+    test = tests_load_url[i];
+
+    url = get_uri_for_path ("/");

Where is this used?

@@ +126,3 @@
+
+    ephy_web_view_load_url (fixture->view, test.url);
+    view_url = ephy_web_view_get_address (fixture->view);

This is actually fascinating. You don't need to run the mainloop at all for that to work? :S

@@ +150,3 @@
+  }
+
+  server = soup_server_new (SOUP_SERVER_PORT, 0, NULL);

You probably want to unref the serve rat the end and stuff.

@@ +153,3 @@
+  soup_server_run_async (server);
+
+  base_uri = soup_uri_new ("http://127.0.0.1/");

and this...?

Created attachment 207021 [details] [review]
tests: add ephy-web-view test

Currently only testing ephy_web_view_load_url

=

Comments applied. Removed Fixture and SoupServer since they are unused.

I honestly doubt this is testing anything at all. It probably just temporarily sets the URL in the view in preparation for loading the page, and unless you run the mainloop and have a web server replying you are not doing anything else. So this does not seem very useful as a test. Am I wrong?

Review of attachment 207021 [details] [review]:

Let's do a full page load test.

Created attachment 207222 [details] [review]
ephy-web-view: share non-search-regex for testing

=

Needed for more extensive testing.

Created attachment 207223 [details] [review]
tests: add ephy-web-view test

=

Ok here is an improved version of this test.
It now covers:

- ephy_web_view_load_url with a GMainLoop to check the actual results.

- the GRegex to determine if a string is non-searchable


The GRegex is an important part of the puzzle to have proper loading.
There are a few test cases that don't pass with the current GRegex.

Will remove TEST_EPHY_WEB_VIEW_NON_SEARCH_REGEX for commit.

Review of attachment 207222 [details] [review]:

This should go in ephy-private.h

Review of attachment 207223 [details] [review]:

Looks good, a few nitpicks.

::: tests/ephy-web-view-test.c
@@ +79,3 @@
+  g_assert_cmpstr (loaded_url, ==, expected_url);
+
+  g_signal_handlers_disconnect_by_func (view, notify_load_status_cb, loop);

Perhaps it's better to just use the same view always, set up in the setup_fixture stuff for tests, to avoid having to do this all the time.

@@ +144,3 @@
+    EphyWebView *view;
+
+    view = EPHY_WEB_VIEW (ephy_web_view_new ());

This is kinda leaked, right?

@@ +146,3 @@
+    view = EPHY_WEB_VIEW (ephy_web_view_new ());
+    test = test_load_url[i];
+    loop = g_main_loop_new (NULL, TRUE);

Not really important, but you want FALSE there, not TRUE.

@@ +223,3 @@
+
+  regex = g_regex_new (EPHY_WEB_VIEW_NON_SEARCH_REGEX,
+                       G_REGEX_OPTIMIZE, G_REGEX_MATCH_NOTEMPTY, &error);

Nitpick, but optimizing the regex for a test probably does not make a lot of sense.

@@ +259,3 @@
+  _ephy_shell_create_instance (FALSE);
+
+  if (!ephy_file_helpers_init (NULL, TRUE, FALSE, NULL)) {

This should really go before you create the shell instance... if it does not make any difference then it's probably useless for testing purposes.

Idea: make a private ephy_test_init that does everything needed, and use it.

Created attachment 208420 [details] [review]
tests: add ephy-web-view test

Updated with your comments.

Re: reusing the web-view is not a big gain IMO. Plus it might give bogus
results if URL loading gets somewhat queued. Consider that I am not
precisely based on evidence for this affirmation.

I guess the ref_sink+unref combo is fine, right?

(In reply to comment #9)
> Review of attachment 207222 [details] [review]:
> 
> This should go in ephy-private.h

ephy-private.h is in src/, AFAIR we avoid depending on stuff from src/

Should I create a embed/ephy-embed-private.h to be included by src/ephy-private.h?

(In reply to comment #12)
> ephy-private.h is in src/, AFAIR we avoid depending on stuff from src/
> 
> Should I create a embed/ephy-embed-private.h to be included by
> src/ephy-private.h?

Yep, exactly that.

Created attachment 208817 [details] [review]
ephy-web-view: share non-search-regex for testing

Add ephy-embed-private.h
=

Should be good now.

Review of attachment 208817 [details] [review]:

Looks good.

Review of attachment 208817 [details] [review]:

Maybe ephy-private.h should include ephy-embed-private.h too, for completeness.

Review of attachment 208420 [details] [review]:

::: tests/ephy-web-view-test.c
@@ +203,3 @@
+                                        "^localhost(\\.[^[:space:]]+)?(:\\d+)?(/.*)?$|" \
+
+#endif

What?

@@ +215,3 @@
+                                        "^file:.*$" \
+                                        ")"
+

I thought the point was to include this from the private header?

(In reply to comment #17)
> Review of attachment 208420 [details] [review]:
> 
> ::: tests/ephy-web-view-test.c
> @@ +203,3 @@
> +                                       
> "^localhost(\\.[^[:space:]]+)?(:\\d+)?(/.*)?$|" \
> +
> +#endif
> 
> What?
> 
> @@ +215,3 @@
> +                                        "^file:.*$" \
> +                                        ")"
> +
> 
> I thought the point was to include this from the private header?

Oops. Forgot to remove that. Those lines are just to be removed, they are not used currently.
I was using that define just to test some tweaks to the regexp.

Created attachment 208873 [details] [review]
tests: add ephy-web-view test

Testing ephy_web_view_load_url and the internal GRegex of EphyWebView.

Review of attachment 208873 [details] [review]:

Go.

Done!

Attachment 208817 [details] pushed as 643d1fd - ephy-web-view: share non-search-regex for testing
Attachment 208873 [details] pushed as b56ec17 - tests: add ephy-web-view test