You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by iilyak <gi...@git.apache.org> on 2015/10/06 17:27:52 UTC
[GitHub] couchdb-couch-mrview pull request: Use couch_server:delete_file on...
GitHub user iilyak opened a pull request:
https://github.com/apache/couchdb-couch-mrview/pull/33
Use couch_server:delete_file on view cleanup
This PR depends on:
- https://github.com/apache/couchdb-couch/pull/115
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloudant/couchdb-couch-mrview rename_on_delete
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-couch-mrview/pull/33.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #33
----
commit 66fb4f29ce74ddd28e8754fa8c3e0b71d0d6732b
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2015-10-06T15:08:17Z
Use couch_server:delete_file on view cleanup
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request: Use couch_server:delete_file on...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/33#discussion_r41408967
--- Diff: src/couch_mrview_cleanup.erl ---
@@ -41,7 +41,71 @@ run(Db) ->
lists:foreach(fun(FN) ->
couch_log:debug("Deleting stale view file: ~s", [FN]),
- couch_file:delete(RootDir, FN, false)
+ delete_view_file(RootDir, FN)
end, ToDelete),
ok.
+
+delete_view_file(RootDir, FullFilePath) ->
+ couch_server:delete_file(RootDir, FullFilePath, [sync]).
+
+-ifdef(TEST).
+-include_lib("couch/include/couch_eunit.hrl").
+
+setup(rename) ->
+ setup(true);
+setup(delete) ->
+ setup(false);
+setup(RenameOnDelete) ->
+ meck:new(config, [passthrough]),
+ meck:expect(config, get_boolean, fun
+ ("couchdb", "rename_on_delete", _Default) -> RenameOnDelete
+ end),
+ ViewFile = ?tempfile() ++ ".view",
+ RootDir = filename:dirname(ViewFile),
+ ok = couch_file:init_delete_dir(RootDir),
+ ok = file:write_file(ViewFile, <<>>),
+ {RootDir, ViewFile}.
+
+teardown(_, {_, ViewFile}) ->
+ (catch meck:unload(config)),
+ (catch file:delete(ViewFile)).
+
+delete_view_file_test_() ->
+ Funs = [
+ fun should_rename_on_delete/2,
+ fun should_delete/2
+ ],
+ [
+ make_test_case(rename, [fun should_rename_on_delete/2]),
+ make_test_case(delete, [fun should_delete/2])
+ ].
+
+make_test_case(Mod, Funs) ->
+ {
+ lists:flatten(io_lib:format("~s", [Mod])),
+ {foreachx, fun setup/1, fun teardown/2, [{Mod, Fun} || Fun <- Funs]}
+ }.
+
+should_rename_on_delete(_, {RootDir, ViewFile}) ->
+ ?_test(begin
+ ?assert(filelib:is_regular(ViewFile)),
+ Result = delete_view_file(RootDir, ViewFile),
+ ?assertMatch({ok, {renamed, _}}, Result),
+ {ok, {renamed, RenamedViewFile}} = Result,
+ ?assertNot(filelib:is_regular(ViewFile)),
+ ?assert(filelib:is_regular(RenamedViewFile))
--- End diff --
much clearer.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request: Use couch_server:delete_file on...
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/33#issuecomment-145902081
Why in-module tests? MRView has separate test suites for the rest of the modules.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request: Use couch_server:delete_file on...
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/33#issuecomment-145913754
@iilyak: ah, fair enough. I suppose we could've use something like `{eunit_compile_opts, [export_all]}` in rebar config to not to mix things, but this is out of the scope.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request: Use couch_server:delete_file on...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/33#discussion_r41280730
--- Diff: src/couch_mrview_cleanup.erl ---
@@ -41,7 +41,52 @@ run(Db) ->
lists:foreach(fun(FN) ->
couch_log:debug("Deleting stale view file: ~s", [FN]),
- couch_file:delete(RootDir, FN, false)
+ delete_view_file(RootDir, FN)
end, ToDelete),
ok.
+
+delete_view_file(RootDir, FullFilePath) ->
+ couch_server:delete_file(RootDir, FullFilePath, [sync]).
+
+-ifdef(TEST).
+-include_lib("couch/include/couch_eunit.hrl").
+
+setup(RenameOnDelete) ->
+ meck:new(config, [passthrough]),
+ meck:expect(config, get_boolean, fun
+ ("couchdb", "rename_on_delete", _Default) -> RenameOnDelete
+ end),
+ ViewFile = ?tempfile() ++ ".view",
+ RootDir = filename:dirname(ViewFile),
+ ok = couch_file:init_delete_dir(RootDir),
+ ok = file:write_file(ViewFile, <<>>),
+ {RootDir, ViewFile}.
+
+remove_on_delete_test() ->
+ try
+ {RootDir, ViewFile} = setup(true),
+ ?assert(filelib:is_regular(ViewFile)),
+ delete_view_file(RootDir, ViewFile),
+ ?assert(not filelib:is_regular(ViewFile)),
+ RenamedViewFile = couch_server:renamed_filename(ViewFile),
+ ?assert(filelib:is_regular(RenamedViewFile)),
+ ok
+ after
+ meck:unload(config)
+ end.
+
+regular_delete_test() ->
+ try
+ {RootDir, ViewFile} = setup(false),
+ ?assert(filelib:is_regular(ViewFile)),
+ delete_view_file(RootDir, ViewFile),
+ ?assert(not filelib:is_regular(ViewFile)),
+ RenamedViewFile = couch_server:renamed_filename(ViewFile),
+ ?assert(not filelib:is_regular(RenamedViewFile)),
+ ok
+ after
+ meck:unload(config)
--- End diff --
can we use setup/teardown in the usual eunit way?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request: Use couch_server:delete_file on...
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/33#issuecomment-145906805
@eiri: > Why in-module tests? MRView has separate test suites for the rest of the modules.
It is in the module tests because I'm testing private function.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request: Use couch_server:delete_file on...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/33#issuecomment-146243943
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request: Use couch_server:delete_file on...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/33#discussion_r41280890
--- Diff: src/couch_mrview_cleanup.erl ---
@@ -41,7 +41,52 @@ run(Db) ->
lists:foreach(fun(FN) ->
couch_log:debug("Deleting stale view file: ~s", [FN]),
- couch_file:delete(RootDir, FN, false)
+ delete_view_file(RootDir, FN)
end, ToDelete),
ok.
+
+delete_view_file(RootDir, FullFilePath) ->
+ couch_server:delete_file(RootDir, FullFilePath, [sync]).
+
+-ifdef(TEST).
+-include_lib("couch/include/couch_eunit.hrl").
+
+setup(RenameOnDelete) ->
+ meck:new(config, [passthrough]),
+ meck:expect(config, get_boolean, fun
+ ("couchdb", "rename_on_delete", _Default) -> RenameOnDelete
+ end),
+ ViewFile = ?tempfile() ++ ".view",
+ RootDir = filename:dirname(ViewFile),
+ ok = couch_file:init_delete_dir(RootDir),
+ ok = file:write_file(ViewFile, <<>>),
+ {RootDir, ViewFile}.
+
+remove_on_delete_test() ->
+ try
+ {RootDir, ViewFile} = setup(true),
+ ?assert(filelib:is_regular(ViewFile)),
+ delete_view_file(RootDir, ViewFile),
+ ?assert(not filelib:is_regular(ViewFile)),
--- End diff --
use `?assertNot(`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request: Use couch_server:delete_file on...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/couchdb-couch-mrview/pull/33
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request: Use couch_server:delete_file on...
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/33#issuecomment-146254318
squashed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch-mrview pull request: Use couch_server:delete_file on...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the pull request:
https://github.com/apache/couchdb-couch-mrview/pull/33#issuecomment-145932141
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---