GNOME Bugzilla – Bug 673337
tests: add ephy-file-helpers-test
Last modified: 2012-05-24 07:48:58 UTC
WIP test for ephy-file-helpers. I am attaching early because of the fixups I have been doing in ephy-file-helpers, see them attached. I still want to test (ordered by priority): ephy_file_delete_uri ephy_file_tmp_filename ephy_file ephy_file_desktop_dir ephy_file_switch_temp_file ephy_file_find Suggestions welcome, reviews -at least for the fixups- are appreciated.
Created attachment 211103 [details] [review] e-file-helpers: remove ephy_file_add_recent_item It's a two-line save, and there are no users of this API.
Created attachment 211104 [details] [review] e-file-helpers: unset EPHY_UUID_ENVVAR on shutdown
Created attachment 211105 [details] [review] e-file-helpers: simplify ephy_file_get_downloads_dir Remove the internal ephy_download_dir and better explain the logic of the function in the comment.
Created attachment 211106 [details] [review] e-file-helpers: prevent invalid enumerator and return value Invalid enumerators can be returned even when no GError is set. Check if the enumerator is non-NULL before proceeding, and adjust the default return value so it is not TRUE when g_file_enumerate_children fails.
Created attachment 211107 [details] [review] tests: add ephy-file-helpers-test Tests init/shutdown process, ephy_get_downloads_dir.
Review of attachment 211103 [details] [review]: OK.
Review of attachment 211104 [details] [review]: What's the point of this exactly? If the process is going away its env. will die anyway, or?
Review of attachment 211105 [details] [review]: I think the comment is OK but honestly I think the refactoring goes in the wrong direction. That method colud be useful elsewhere and I see no point in merging it in the function that calls it. ::: lib/ephy-file-helpers.c @@ +115,3 @@ + * ephy_file_desktop_dir(). + * + * - "Downloads keyword in GSettings, or any other value: the XDG Missing " after Downloads.
(In reply to comment #7) > Review of attachment 211104 [details] [review]: > > What's the point of this exactly? If the process is going away its env. will > die anyway, or? Be able to call init() and shutdown() multiple times in the tests. Shutdown cleans up directories, so you can not pretend nothing happened.
(In reply to comment #8) > Review of attachment 211105 [details] [review]: > > I think the comment is OK but honestly I think the refactoring goes in the > wrong direction. That method colud be useful elsewhere and I see no point in > merging it in the function that calls it. > Ok, makes sense. Will update as only the comment.
Review of attachment 211106 [details] [review]: Looks good.
Created attachment 211487 [details] [review] e-file-helpers: simplify ephy_file_get_downloads_dir Better explain the logic of the function and reorder the conditions. This makes ~/Downloads the fallback instead of ~/Desktop.
Attachment 211103 [details] pushed as d337ce9 - e-file-helpers: remove ephy_file_add_recent_item Attachment 211106 [details] pushed as 5058955 - e-file-helpers: prevent invalid enumerator and return value
Created attachment 211491 [details] [review] e-file-helpers: improve ephy_file_tmp_filename comment
Created attachment 211512 [details] [review] tests: add ephy-file-helpers-test Tests everything except ephy_file and ephy_file_find. I don't think those two are worth it. Only two or three uses in the codebase and hard to get wrong.
Also opened: bug #673666 and bug #673665. Minor catches not directly related to the test.
Review of attachment 211487 [details] [review]: Are we changing the default because of what our schema file says or?
Review of attachment 211491 [details] [review]: OK.
Review of attachment 211512 [details] [review]: Looks good in general. ::: tests/ephy-file-helpers-test.c @@ +294,3 @@ + char *file_cont = NULL; + + ephy_file_helpers_init (NULL, TRUE, FALSE, NULL); I just realized this API sucks ass. Oh well, it wasn't mean to be called more than once on init ... @@ +363,3 @@ + * g_get_user_special_dir. The values there are the ones we should + * check for in the test. */ + g_setenv ("XDG_CONFIG_HOME", TOP_SRC_DIR "/tests/data/", TRUE); Uhm, so where is this using user-dirs.dirs exactly? I guess it happens automatically if the file is there? @@ +391,3 @@ + ephy_file +#endif + Kill with fire.
(In reply to comment #17) > Review of attachment 211487 [details] [review]: > > Are we changing the default because of what our schema file says or? Right now we have: - Downloads keyword → XDG downloads dir - Desktop keyword OR anything else not a valid path → XDG desktop dir - Valid paths → use as-is It is changing to: - Desktop keyword → XDG desktop dir - Downloads keyword OR anything else not a valid path → XDG downloads dir - Valid paths → use as-is So what changes is our fallback. The default is still Downloads as per the schema.
(In reply to comment #19) > Review of attachment 211512 [details] [review]: > > Looks good in general. > > ::: tests/ephy-file-helpers-test.c > @@ +294,3 @@ > + char *file_cont = NULL; > + > + ephy_file_helpers_init (NULL, TRUE, FALSE, NULL); > > I just realized this API sucks ass. Oh well, it wasn't mean to be called more > than once on init ... > Agree, I was thinking on changing those booleans for flags or something less cryptic than "TRUE something". > @@ +363,3 @@ > + * g_get_user_special_dir. The values there are the ones we should > + * check for in the test. */ > + g_setenv ("XDG_CONFIG_HOME", TOP_SRC_DIR "/tests/data/", TRUE); > > Uhm, so where is this using user-dirs.dirs exactly? I guess it happens > automatically if the file is there? Yes, this makes glib use our test configuration when using the g_get_user_special_dir API. The XDG_CONFIG_HOME var should point to the directory with user-dirs.dirs > @@ +391,3 @@ > + ephy_file > +#endif > + > > Kill with fire. BURN
(In reply to comment #7) > Review of attachment 211104 [details] [review]: > > What's the point of this exactly? If the process is going away its env. will > die anyway, or? This needs to go in before the test because of the shutdown() calls. The problem is that the env var is not unset, so you can not init/shutdown more than once in the same process.
Also, a small fix also in this branch (remove old dirs): bug #673665
Comment on attachment 211491 [details] [review] e-file-helpers: improve ephy_file_tmp_filename comment Still insisting that @211104 - e-file-helpers: unset EPHY_UUID_ENVVAR on shutdown is needed for the test to pass. Otherwise it can not init/shutdown the file helpers API with different flags. Also, consider the comment re: downloads_dir patch. Attachment 211491 [details] pushed as a101be1 - e-file-helpers: improve ephy_file_tmp_filename comment
Review of attachment 211104 [details] [review]: OK then. I suspect this is already broken anyway wrt the "launch ephy from within ephy" anyway..
Attachment 211104 [details] pushed as cebb6d5 - e-file-helpers: unset EPHY_UUID_ENVVAR on shutdown Attachment 211512 [details] pushed as d1ba46b - tests: add ephy-file-helpers-test -- Committed. What should I do with the downloads one? See comment #20.
I had to revert the unit test, it fails on distcheck (as usual :/) and I have to release *now*.
So after reviewing the failure of the test, it is because the non applied Attachment 211487 [details]. Without it, the fallback for ephy_file_get_downloads_dir() is XDG_DESKTOP_DIR instead of XDG_DOWNLOADS_DIR. So, it's either doing a replace in the test, or committing that patch.
Review of attachment 211487 [details] [review]: OK then.
Pushed and updated test and the downloads dir API patch. Migrator test is failing, btw. Confirmed that make check was working. Attachment 211487 [details] pushed as b3804ab - e-file-helpers: simplify ephy_file_get_downloads_dir
(In reply to comment #30) > Pushed and updated test and the downloads dir API patch. > > Migrator test is failing, btw. Confirmed that make check was working. Failing how? distcheck passes here (without your patches, commit 02e9b253)