GNOME Bugzilla – Bug 613756
add test for ephy-embed-utils has-web-scheme
Last modified: 2012-02-08 05:28:09 UTC
We need more tests!
Created attachment 158568 [details] [review] Tests for ephy-embed-utils Test for ephy-embed-utils/address_has_web_scheme. Note that the test currently fail, as address_has_web_scheme returns TRUE for strings starting on ":". I don't know how to test link_message_parse, as it uses i18n functions, so it will return different values depending on locale. Tests for normalize_web_address are on bug #612078.
Created attachment 158569 [details] [review] Bugfix for ephy-embed-utils/has_web_scheme Simple correction for has_web_scheme, that returns true for strings starting on ':'.
Created attachment 158572 [details] [review] Tests for ephy-embed-utils Declare strings as const.
Created attachment 159741 [details] [review] Tests for ephy-embed-utils Patch using git format-patch
Created attachment 159742 [details] [review] Bugfix for ephy-embed-utils/has_web_scheme Patch using git format-patch
Review of attachment 159742 [details] [review]: Looks good to me. Specially with the test to validate your change :). Xan?
Review of attachment 159741 [details] [review]: Cool. Seems simple enough. I think it's oktc, but first let's agree on the one liner, which I also think looks safe.
Created attachment 159744 [details] [review] Another test for ephy-embed-utils Found another failing test for has_web_scheme, so my previous fix is wrong.
Created attachment 159746 [details] [review] Fix for ephy-embed-utils/has_web_scheme Not so simple as the previous one, but passes the tests.
Created attachment 160871 [details] [review] Remove unused variable
Created attachment 160872 [details] [review] Add tests to incomplete schemes Test to make sure all lengths are correct.
Comment on attachment 160871 [details] [review] Remove unused variable You should have removed colonpos and never add colon. See your previous patch.
Comment on attachment 159746 [details] [review] Fix for ephy-embed-utils/has_web_scheme You forgot to remove colonpos and added colon.
Patches LGTM, I'd commit them with a few nitpicks in the commit messages for tests/: add the "tests:" prefix for your two patches touching testephyembedutils.c, just like the ephy-embed-utils: prefix.
Created attachment 173974 [details] [review] tests: add test for ephy-embed-utils Specifically ephy_embed_utils_address_has_web_scheme. Bug #613756
Created attachment 173975 [details] [review] ephy-embed-utils: prevent false positives in has_web_scheme Bug #613756
Review of attachment 173974 [details] [review]: ::: tests/testephyembedutils.c @@ +1,1 @@ +/* vim: set sw=2 ts=2 sts=2 et: */ only the vim ones? uh. @@ +4,3 @@ + * This file is part of Epiphany + * + * Copyright © 2010 - Alexandre Martani What? @@ +71,3 @@ + g_assert (!ephy_embed_utils_address_has_web_scheme (without_web_scheme[i])); + } +} No braces for one line control clauses, says HACKING.
Review of attachment 173975 [details] [review]: It's not immediately obvious to me here what false positive you are fixing. Care to elaborate and put it in the commit message?
Created attachment 175138 [details] [review] tests: add test for ephy-embed-utils Specifically ephy_embed_utils_address_has_web_scheme. Bug #613756
Created attachment 175139 [details] [review] ephy-embed-utils: improve has_web_scheme Fix some false positives and improve ephy_embed_utils_address_has_web_scheme(): - returns FALSE if there's no colon before the 11th character - correctly handle g_ascii_strncasecmp() return value with == 0 - hardcode colon position for each scheme - avoid duplicated lines with an array of schemes Based on a patch by Alexandre Martani. Bug #613756
Re: your comments: - the test is © Alexandre because he wrote it - style fixed in test - I rewrote the embed-utils patch almost from scratch, commit msg has a list of what's fixed, do you like the struct or is it too much?
Created attachment 206012 [details] [review] tests: add test for ephy-embed-utils Specifically ephy_embed_utils_address_has_web_scheme. Based on a patch by Alexandre Martani.
Review of attachment 206012 [details] [review]: This is OK, but what we need to test is that loading the URIs in an EphyWebView actually starts a load. That's what usually breaks, not this util method. ::: tests/ephy-embed-utils.c @@ +5,3 @@ + * This file is part of Epiphany + * + * Copyright © 2011 Igalia S.L. 2012 @@ +24,3 @@ + +#include "config.h" +#include "ephy-embed-utils.h" An empty line here. @@ +43,3 @@ + { "data", "data:\%20\%40" }, + { "about", "about:epiphany" }, + { "epiphany-about", "epiphany-about:memory" }, It's ephy-about, not epiphany-about. Otherwise it will fail.
Created attachment 206197 [details] [review] tests: add test for ephy-embed-utils Specifically ephy_embed_utils_address_has_web_scheme. Based on a patch by Alexandre Martani.
Created attachment 206198 [details] [review] ephy-embed-utils: improve has_web_scheme Fix some false positives and improve ephy_embed_utils_address_has_web_scheme(): - returns FALSE if there's no colon before the 11th character - correctly handle g_ascii_strncasecmp() return value with == 0 - hardcode colon position for each scheme - avoid duplicated lines with an array of schemes Based on a patch by Alexandre Martani.
Created attachment 206199 [details] [review] ephy-embed-utils: cleanup normalize_address Correctly handle g_strcmp0 return value, simplify the conditionals.
Review of attachment 206198 [details] [review]: ::: embed/ephy-embed-utils.c @@ +112,3 @@ + schemes[i].prefix, + schemes[i].colpos) == 0); + } Hum, just break early if you find it? You are also kinda losing the small optimization of returning FALSE if there's no colon before the 11th character.
Review of attachment 206197 [details] [review]: ::: tests/Makefile.am @@ +112,3 @@ +test_ephy_embed_utils_SOURCES = \ + ephy-embed-utils.c I prefer test-ephy-embed-utils.c, otherwise there's duplicated source file names in the tree. ::: tests/ephy-embed-utils.c @@ +5,3 @@ + * This file is part of Epiphany + * + * Copyright © 2011 Igalia S.L. 2012. @@ +31,3 @@ + const char *name; + const char *test; +} SchemeTest; I honestly have no idea of what 'const char*' means in a struct.
Review of attachment 206199 [details] [review]: - I don't see how g_strcmp0 was broken before (?) - Not immediately obvious how you are simplifying things, since you are adding much more stuff to the method, needs more explanation. - There are still no unit tests that check that loading pages in a webview actually works, so I wouldn't keep touching this code after our last experience here.
(In reply to comment #28) > Review of attachment 206197 [details] [review]: > > ::: tests/Makefile.am > @@ +112,3 @@ > > +test_ephy_embed_utils_SOURCES = \ > + ephy-embed-utils.c > > I prefer test-ephy-embed-utils.c, otherwise there's duplicated source file > names in the tree. That means renaming the other tests too. My original reason for this name scheme was that tab completion on tests/ was a pest, both binaries and files were offered. > ::: tests/ephy-embed-utils.c > @@ +5,3 @@ > + * This file is part of Epiphany > + * > + * Copyright © 2011 Igalia S.L. > > 2012. :P > @@ +31,3 @@ > + const char *name; > + const char *test; > +} SchemeTest; > > I honestly have no idea of what 'const char*' means in a struct. This is me, not knowing much C. Educate me.
(In reply to comment #29) > Review of attachment 206199 [details] [review]: > > - I don't see how g_strcmp0 was broken before (?) > - Not immediately obvious how you are simplifying things, since you are adding > much more stuff to the method, needs more explanation. > - There are still no unit tests that check that loading pages in a webview > actually works, so I wouldn't keep touching this code after our last experience > here. Agree. I'll work on an additional test for EphyWebView so we can test this at both levels. From that I'll see if the changes still make sense.
(In reply to comment #30) > > I prefer test-ephy-embed-utils.c, otherwise there's duplicated source file > > names in the tree. > > That means renaming the other tests too. My original reason for this name > scheme was that tab completion on tests/ was a pest, both binaries and files > were offered. I see, but I think having duplicated filenames is really bad (sucks while you are worknig on the test, and if we ever move to non-recursive Makefiles it will bite us in the ass I think). ephy-embed-utils-test.c ? > > @@ +31,3 @@ > > + const char *name; > > + const char *test; > > +} SchemeTest; > > > > I honestly have no idea of what 'const char*' means in a struct. > > This is me, not knowing much C. Educate me. What I meant is, "why are you doing that?". I'm not sure it has any useful effect in this context :)
(In reply to comment #32) > (In reply to comment #30) > > > I prefer test-ephy-embed-utils.c, otherwise there's duplicated source file > > > names in the tree. > > > > That means renaming the other tests too. My original reason for this name > > scheme was that tab completion on tests/ was a pest, both binaries and files > > were offered. > > I see, but I think having duplicated filenames is really bad (sucks while you > are worknig on the test, and if we ever move to non-recursive Makefiles it will > bite us in the ass I think). ephy-embed-utils-test.c ? > Ok, that sounds nicer than tab frustration :-P. I will write a simple patch for that. Btw, I already have a simple ephy-web-view test. I will include it in the next batch of patches. > > > @@ +31,3 @@ > > > + const char *name; > > > + const char *test; > > > +} SchemeTest; > > > > > > I honestly have no idea of what 'const char*' means in a struct. > > > > This is me, not knowing much C. Educate me. > > What I meant is, "why are you doing that?". I'm not sure it has any useful > effect in this context :) AFAIK because those are strings I'm not going to change, free, modify or whatever. So I guess it's just a micro micro micro optimization?
Created attachment 206959 [details] [review] tests: add test for ephy-embed-utils Specifically ephy_embed_utils_address_has_web_scheme. Based on a patch by Alexandre Martani.
Review of attachment 206959 [details] [review]: OK.
Done! Hopefully we will fix this soon. Attachment 206959 [details] pushed as a65a772 - tests: add test for ephy-embed-utils