Add post-{commit,merge} #857

arielshaqed · 2020-10-21T14:05:53Z

Part of #534 but will be useful elsewhere. Allows loosely-coupled components, will be used to
support export.

codecov-io · 2020-10-21T14:14:38Z

Codecov Report

Merging #857 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #857      +/-   ##
==========================================
+ Coverage   45.05%   45.09%   +0.03%     
==========================================
  Files         140      140              
  Lines       10842    10853      +11     
==========================================
+ Hits         4885     4894       +9     
- Misses       5325     5326       +1     
- Partials      632      633       +1

Impacted Files	Coverage Δ
catalog/cataloger.go	`69.28% <100.00%> (+1.13%)`	⬆️
catalog/cataloger_commit.go	`76.81% <100.00%> (-1.98%)`	⬇️
catalog/cataloger_merge.go	`62.96% <100.00%> (+0.69%)`	⬆️
catalog/cataloger_create_entry.go	`94.73% <0.00%> (-5.27%)`	⬇️
catalog/cataloger_create_repository.go	`65.38% <0.00%> (+3.84%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a8ca25...5000139. Read the comment docs.

nopcoder · 2020-10-21T14:13:32Z

catalog/cataloger_merge_test.go

-		{Path: newFilename},
-		{Path: overFilename, Seed: "seed1"},
-		{Path: delFilename, Deleted: true},
+func validateMergeWithHooks(


a bit too much, no? a simple test of any simple merge use case should verify that hooks works - looking for an issue on a failed test that is written like that will make it hard on the developer.

Isn't this the simple merge case? It requires creating 2 branches, creating an entry on one, merging, and then verifying that the hook was called (first case) and also that the hook can fail merging.

Rewriting as a separate pair of tests. It keeps this test shorter, but means much more test code to understand. Worst case you can ask for the old code back.

nopcoder · 2020-10-21T14:15:56Z

catalog/cataloger_commit_test.go

+	c := testCataloger(t)
+
+	t.Run("commit hooks run and see commit", func(t *testing.T) {
+		repository := testCatalogerRepo(t, ctx, c, "repository", "master")


you can take out of the repo creation out of both tests

Every other test does that too. We can refactor in a separate PR.

catalog/cataloger_commit_test.go

nopcoder · 2020-10-21T14:20:52Z

catalog/cataloger.go

@@ -152,6 +152,10 @@ type Merger interface {
 	Merge(ctx context.Context, repository, leftBranch, rightBranch, committer, message string, metadata Metadata) (*MergeResult, error)
 }

+type Hookser interface {
+	GetHooks() *CatalogerHooks


Suggested change

GetHooks() *CatalogerHooks

Hooks() *CatalogerHooks

itaiad200 · 2020-10-21T14:38:47Z

catalog/cataloger.go

+// called in a current transaction context; if they return an error the transaction is rolled
+// back.  Because these transactions are current, the hook can see the effect the operation only
+// on the passed transaction.
+type CatalogerHooks struct {


Maybe add a branch filter by name/regex?
o.w. you're relying on the hook itself to filter by it.

Shall not do this. If needed in >1 hooks, we could add a wrapper ("decorator") function that filters a hook implementation for regexp. The added complexity of doing it here would include special semantics for merges -- there are two branches here.
Finally, I'm not sure it will be useful. For exports it's not, because it is hard to precompute the regexp (changes transactionally with the DB) and easy to check directly on the DB.

arielshaqed

Thanks!

I think I covered everything. Let me know if you would like me to roll back the new post-merge hook tests, I don't particularly like them either way (old or new).

arielshaqed · 2020-10-22T07:02:44Z

catalog/cataloger.go

@@ -152,6 +152,10 @@ type Merger interface {
 	Merge(ctx context.Context, repository, leftBranch, rightBranch, committer, message string, metadata Metadata) (*MergeResult, error)
 }

+type Hookser interface {
+	GetHooks() *CatalogerHooks


arielshaqed · 2020-10-22T07:05:39Z

catalog/cataloger.go

+// called in a current transaction context; if they return an error the transaction is rolled
+// back.  Because these transactions are current, the hook can see the effect the operation only
+// on the passed transaction.
+type CatalogerHooks struct {


Shall not do this. If needed in >1 hooks, we could add a wrapper ("decorator") function that filters a hook implementation for regexp. The added complexity of doing it here would include special semantics for merges -- there are two branches here.
Finally, I'm not sure it will be useful. For exports it's not, because it is hard to precompute the regexp (changes transactionally with the DB) and easy to check directly on the DB.

arielshaqed · 2020-10-22T07:06:07Z

catalog/cataloger_commit_test.go

+	c := testCataloger(t)
+
+	t.Run("commit hooks run and see commit", func(t *testing.T) {
+		repository := testCatalogerRepo(t, ctx, c, "repository", "master")


Every other test does that too. We can refactor in a separate PR.

arielshaqed · 2020-10-22T07:19:07Z

catalog/cataloger_merge_test.go

-		{Path: newFilename},
-		{Path: overFilename, Seed: "seed1"},
-		{Path: delFilename, Deleted: true},
+func validateMergeWithHooks(


Isn't this the simple merge case? It requires creating 2 branches, creating an entry on one, merging, and then verifying that the hook was called (first case) and also that the hook can fail merging.

Rewriting as a separate pair of tests. It keeps this test shorter, but means much more test code to understand. Worst case you can ask for the old code back.

Part of #534 but will be useful elsewhere. Allows loosely-coupled components, will be used to support export.

Co-authored-by: Barak Amar <barak.amar@treeverse.io>

Also explicitly take a test name in `testCreateEntryCalcChecksum` rather than manually appending it to a seed.

catalog/cataloger_commit_test.go

nopcoder

just suggested an alternative code for the tests, all the rest is cool.

nopcoder · 2020-10-22T11:58:05Z

catalog/cataloger_test.go

@@ -83,9 +85,9 @@ func testCatalogerGetEntry(t testing.TB, ctx context.Context, c Cataloger, repos
 	}
 }

-func testCreateEntryCalcChecksum(key string, seed string) string {
+func testCreateEntryCalcChecksum(key string, testName string, seed string) string {


the plan was to pass the test name as seed if needed and call this function.
possibly calling this function again for verification.
there is no need to contact the test name when we have seed.

The problem is that if any 2 tests use the same key and seed then they clash. It caused an actual problem when I copied calls around.

nopcoder · 2020-10-22T12:00:13Z

catalog/cataloger_merge_test.go

@@ -1200,3 +1201,60 @@ func TestCataloger_MergeFromChildAfterMergeFromParent(t *testing.T) {
 		t.Fatalf("Merge err=%s, expected none", err)
 	}
 }
+
+func TestCataloger_Merge_Hooks(t *testing.T) {


let me know what you think about the diff test code - if you think it is easier to read, update this one - max I can live with both cases or give you the code for this one.

Will copy that code over here, you're correct that it's a bit nicer.

Co-authored-by: Barak Amar <barak.amar@treeverse.io>

arielshaqed

Thanks!

Pulling once tests pass.

arielshaqed · 2020-10-22T12:06:19Z

catalog/cataloger_merge_test.go

@@ -1200,3 +1201,60 @@ func TestCataloger_MergeFromChildAfterMergeFromParent(t *testing.T) {
 		t.Fatalf("Merge err=%s, expected none", err)
 	}
 }
+
+func TestCataloger_Merge_Hooks(t *testing.T) {


Will copy that code over here, you're correct that it's a bit nicer.

arielshaqed · 2020-10-22T12:07:46Z

catalog/cataloger_test.go

@@ -83,9 +85,9 @@ func testCatalogerGetEntry(t testing.TB, ctx context.Context, c Cataloger, repos
 	}
 }

-func testCreateEntryCalcChecksum(key string, seed string) string {
+func testCreateEntryCalcChecksum(key string, testName string, seed string) string {


The problem is that if any 2 tests use the same key and seed then they clash. It caused an actual problem when I copied calls around.

arielshaqed requested a review from nopcoder October 21, 2020 14:05

nopcoder suggested changes Oct 21, 2020

View reviewed changes

itaiad200 reviewed Oct 21, 2020

View reviewed changes

arielshaqed commented Oct 22, 2020

View reviewed changes

arielshaqed requested a review from nopcoder October 22, 2020 11:12

arielshaqed and others added 4 commits October 22, 2020 14:13

Add post-{commit,merge}

1963648

Part of #534 but will be useful elsewhere. Allows loosely-coupled components, will be used to support export.

[CR] Catalog test patches

4538ae7

Co-authored-by: Barak Amar <barak.amar@treeverse.io>

[CR] Use testCreateEntry in more tests

67aa5a2

[CR] Rename GetHooks, add separate merge hooks tests

1d173cb

Also explicitly take a test name in `testCreateEntryCalcChecksum` rather than manually appending it to a seed.

arielshaqed force-pushed the feature/534-hooks branch from a7b1f31 to 1d173cb Compare October 22, 2020 11:14

nopcoder reviewed Oct 22, 2020

View reviewed changes

catalog/cataloger_commit_test.go Outdated Show resolved Hide resolved

nopcoder approved these changes Oct 22, 2020

View reviewed changes

arielshaqed and others added 2 commits October 22, 2020 15:29

[CR] suggestions: better commit test

646e9e0

Co-authored-by: Barak Amar <barak.amar@treeverse.io>

Simplify hook success/failure tests with an external hooks object

5000139

arielshaqed commented Oct 22, 2020

View reviewed changes

arielshaqed merged commit cf0d129 into master Oct 22, 2020

arielshaqed deleted the feature/534-hooks branch October 22, 2020 13:16

arielshaqed restored the feature/534-hooks branch October 25, 2020 11:04

arielshaqed deleted the feature/534-hooks branch October 25, 2020 11:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add post-{commit,merge} #857

Add post-{commit,merge} #857

arielshaqed commented Oct 21, 2020

codecov-io commented Oct 21, 2020 •

edited

nopcoder Oct 21, 2020

arielshaqed Oct 22, 2020

nopcoder Oct 21, 2020

arielshaqed Oct 22, 2020

nopcoder Oct 21, 2020

arielshaqed Oct 22, 2020

itaiad200 Oct 21, 2020

arielshaqed Oct 22, 2020

arielshaqed left a comment

arielshaqed Oct 22, 2020

arielshaqed Oct 22, 2020

arielshaqed Oct 22, 2020

arielshaqed Oct 22, 2020

nopcoder left a comment

nopcoder Oct 22, 2020

arielshaqed Oct 22, 2020

nopcoder Oct 22, 2020

arielshaqed Oct 22, 2020

arielshaqed left a comment

arielshaqed Oct 22, 2020

arielshaqed Oct 22, 2020

Add post-{commit,merge} #857

Add post-{commit,merge} #857

Conversation

arielshaqed commented Oct 21, 2020

codecov-io commented Oct 21, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 21, 2020 •

edited