Re: [PATCH v5 23/24] t1405: some basic tests on main ref store
From: Duy Nguyen <hidden>
Date: 2017-03-06 12:32:52
On Fri, Mar 3, 2017 at 11:43 PM, Michael Haggerty [off-list ref] wrote:
It's notable that these tests grep around the filesystem, so they won't be applicable to future refs backends. Of course, "pack-refs" is intrinsically only applicable to the files backend, so for this test it's not surprising. But some tests could conceivably be written in a generic way, so that they should pass for any refs backend. Just food for thought; no need to change anything now.
I'm a bit on the fence about this. On one hand I think there is room to backend-specific tests (and this one is more about files backend, we just don't have any direct way of getting the backend except through get_main_ref_store()). On the other hand, I can see a need for verifying refs behavior across backends. Submodule backend is unfortunately a bad fit (and probably worktree backend too in early phase) because it cannot fully replace files backend. lmdb does. I guess these tests will have some more restructuring when lmdb joins the party. I imagine we could have something like "ref_expect_success [backend,[backend..]] <title> <body>", which makes it easier to exercise a new backend with the same test, or we could add backend-specific tests as well. Not sure how to do it yet (the devil will be in the body, I think, like dealing with "git -C sub" for submodules). Probably won't do in this series anyway.
quoted
+test_expect_success 'rename_refs(master, new-master)' ' + git rev-parse master >expected && + $RUN rename-ref refs/heads/master refs/heads/new-master && + git rev-parse new-master >actual && + test_cmp expected actual && + test_commit recreate-master +'Isn't HEAD set to `refs/heads/master` prior to this test? If so, then I think it should become detached when you rename `refs/heads/master`. Or maybe it should be changed to point at `refs/heads/new-master`, I can't remember. In either case, it might be useful for the test to check that the behavior matches the status quo, so we notice if the behavior ever changes inadvertently.
You had me worried a bit there, that I broke something. Yes we rename HEAD too. No it's not the backend's job. It's done separately by builtin/branch.c. Probably a good thing because I don't the backend should know about HEAD's special semantics. Front-end might though.
quoted
+test_expect_success 'delete_ref(refs/heads/foo)' ' + SHA1=`git rev-parse foo` && + git checkout --detach && + $RUN delete-ref refs/heads/foo $SHA1 0 && + test_must_fail git rev-parse refs/heads/foo -- +'The last two tests have the same name. You might also want to test these functions when the old oid is incorrect or when the reference is missing.
I will. But you probably noticed that I didn't cover refs behavior extensively. I don't know this area well enough to write a conformant test suite. But I think that's ok. We have something to start improving now. -- Duy