GNOME Bugzilla – Bug 673271
tests: add ephy-web-app-utils-test
Last modified: 2012-06-14 23:27:36 UTC
Tests creation, existance and deletion.
Created attachment 211053 [details] [review] tests: add ephy-web-app-utils-test Tests creation, existance and deletion API. Does not require network access.
Created attachment 211054 [details] [review] e-web-app-utils: use LOG and g_warning instead of g_print
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?
Review of attachment 211054 [details] [review]: This does not seem related at all with this bug.
(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.
(In reply to comment #4) > Review of attachment 211054 [details] [review]: > > This does not seem related at all with this bug. Opened bug #673348
(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.
(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.
Created attachment 215072 [details] [review] e-web-app-utils: warn when app dir already exists
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?
Review of attachment 215072 [details] [review]: OK.
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.
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