After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 673271 - tests: add ephy-web-app-utils-test
tests: add ephy-web-app-utils-test
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: General
unspecified
Other All
: Normal normal
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-04-01 02:01 UTC by Diego Escalante Urrelo (not reading bugmail)
Modified: 2012-06-14 23:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tests: add ephy-web-app-utils-test (5.59 KB, patch)
2012-04-01 02:01 UTC, Diego Escalante Urrelo (not reading bugmail)
reviewed Details | Review
e-web-app-utils: use LOG and g_warning instead of g_print (1.52 KB, patch)
2012-04-01 02:02 UTC, Diego Escalante Urrelo (not reading bugmail)
reviewed Details | Review
e-web-app-utils: warn when app dir already exists (1.06 KB, patch)
2012-05-26 22:14 UTC, Diego Escalante Urrelo (not reading bugmail)
committed Details | Review
tests: add ephy-web-app-utils-test (6.10 KB, patch)
2012-05-26 22:24 UTC, Diego Escalante Urrelo (not reading bugmail)
committed Details | Review

Description Diego Escalante Urrelo (not reading bugmail) 2012-04-01 02:01:44 UTC
Tests creation, existance and deletion.
Comment 1 Diego Escalante Urrelo (not reading bugmail) 2012-04-01 02:01:46 UTC
Created attachment 211053 [details] [review]
tests: add ephy-web-app-utils-test

Tests creation, existance and deletion API. Does not require network
access.
Comment 2 Diego Escalante Urrelo (not reading bugmail) 2012-04-01 02:02:06 UTC
Created attachment 211054 [details] [review]
e-web-app-utils: use LOG and g_warning instead of g_print
Comment 3 Xan Lopez 2012-04-02 10:11:49 UTC
Review of attachment 211053 [details] [review]:

Uhm, so these tests depend on the side effects of the previous tests to work? I'm not an expert in testing but I'm pretty sure this is not a great idea? If one fails all of them will and you won't be sure of what's going on...

::: tests/ephy-embed-single-test.c
@@ +101,3 @@
   ret = g_test_run ();
 
+  g_object_unref (ephy_shell);

What?

::: tests/ephy-web-app-utils-test.c
@@ +136,3 @@
+                   test_web_app_create);
+
+  g_test_add_func ("/embed/ephy-web-app-utils/exist",

exists?
Comment 4 Xan Lopez 2012-04-02 10:12:43 UTC
Review of attachment 211054 [details] [review]:

This does not seem related at all with this bug.
Comment 5 Diego Escalante Urrelo (not reading bugmail) 2012-04-02 10:43:05 UTC
(In reply to comment #3)
> Review of attachment 211053 [details] [review]:
> 
> Uhm, so these tests depend on the side effects of the previous tests to work?
> I'm not an expert in testing but I'm pretty sure this is not a great idea? If
> one fails all of them will and you won't be sure of what's going on...
> 

I might have been a bit lazy with that. Also, I forgot tests can run independently. Will update.

> ::: tests/ephy-embed-single-test.c
> @@ +101,3 @@
>    ret = g_test_run ();
> 
> +  g_object_unref (ephy_shell);
> 
> What?
> 

Wrongly merged this change into this patch. This is the change that causes bug #673273.
Comment 6 Diego Escalante Urrelo (not reading bugmail) 2012-04-02 10:52:02 UTC
(In reply to comment #4)
> Review of attachment 211054 [details] [review]:
> 
> This does not seem related at all with this bug.

Opened bug #673348
Comment 7 Diego Escalante Urrelo (not reading bugmail) 2012-04-02 10:58:46 UTC
(In reply to comment #5)
> (In reply to comment #3)
> > Review of attachment 211053 [details] [review] [details]:
> > 
> > Uhm, so these tests depend on the side effects of the previous tests to work?
> > I'm not an expert in testing but I'm pretty sure this is not a great idea? If
> > one fails all of them will and you won't be sure of what's going on...
> > 
> 
> I might have been a bit lazy with that. Also, I forgot tests can run
> independently. Will update.

Thinking twice after a quick look at the code: do you think it is more reasonable to accept the dependence between this API and just have one big test function? With proper g_assert coverage it should be as safe as 3 different functions. I guess.
Comment 8 Xan Lopez 2012-04-26 19:12:53 UTC
(In reply to comment #7)
> Thinking twice after a quick look at the code: do you think it is more
> reasonable to accept the dependence between this API and just have one big test
> function? With proper g_assert coverage it should be as safe as 3 different
> functions. I guess.

This seems more reasonable if we cannot split the tests cleanly.
Comment 9 Diego Escalante Urrelo (not reading bugmail) 2012-05-26 22:14:22 UTC
Created attachment 215072 [details] [review]
e-web-app-utils: warn when app dir already exists
Comment 10 Diego Escalante Urrelo (not reading bugmail) 2012-05-26 22:24:28 UTC
Created attachment 215074 [details] [review]
tests: add ephy-web-app-utils-test

Improved version:
- only one big test, properly covered in asserts
- proper checking of creation, existance of files
- works under private profile prefix, no danger to current user data

What do you think?
Comment 11 Xan Lopez 2012-06-14 08:54:09 UTC
Review of attachment 215072 [details] [review]:

OK.
Comment 12 Xan Lopez 2012-06-14 09:03:26 UTC
Review of attachment 215074 [details] [review]:

Looks good, just a tiny nitpick.

::: tests/ephy-web-app-utils-test.c
@@ +91,3 @@
+    /* Test list API */
+    apps = ephy_web_application_get_application_list ();
+    g_assert_cmpint (g_list_length (apps), ==, 1);

Maybe I'm sleepy, but why is this always 1? Aren't we creating 3 web apps in the end?

OK, nevermind, you delete it afterwards. That's a bit confusing! Maybe add a small comment.
Comment 13 Diego Escalante Urrelo (not reading bugmail) 2012-06-14 23:27:30 UTC
Attachment 215072 [details] pushed as 3de7945 - e-web-app-utils: warn when app dir already exists
Attachment 215074 [details] pushed as b6cdb8b - tests: add ephy-web-app-utils-test