refactor: Rework integration test framework #1089

AndrewSisley · 2023-02-09T18:48:36Z

Relevant issue(s)

Resolves #1087
SIP: https://github.com/sourcenetwork/SIPs/pull/8

Description

Introduces a new integration test framework, and migrates the (main) old one to it. The linked SIP goes into why I want to do this, as does some of the commit bodies (to a lesser extent).

I'm not sure how you guys will find it easiest to review this - I broke up the work into two commits, one introduces the new framework, the other migrates the old framework to it. There is some hopefully useful info in those commit message bodies and it might be handy to read both before reviewing either commit.

I have not changed any of the change detector setup code although how the split is handled has changed. The Init code has also not changed. Both have moved. I consider anything in those two sections to be out of scope. Regarding everything else - even though large parts of the rest of the code is just copy-paste it might be a good time to flag any existing oddities there so please feel very free to comment on it even if it looks familiar.

Note: A couple of new types and functions have just the old names suffixed with 2. I'm happy with this for now and they can be changed later as the old types/funcs get removed.

Also, quick note on code coverage as I guess this has a high chance of coming up - the P2P coverage bounces around a bit at the moment and has done since at least the state based tests went in as some lines only get hit depending on what what else is going on at the same time/execution order. This is not caused by the new framework.

I've recently started to convert some tests to the proposed system, if interested you can see them here: https://github.com/sourcenetwork/defradb/compare/sisley/refactor/I1087-migrate-int-tests-1?expand=1

tests/integration/explain/default/delete_test.go

shahzadlone · 2023-02-12T08:05:56Z

question: Do you see any value in the new explain testing setup (which is still in the middle of migrating to the new setup) to adopt similar action / order based setup?

AndrewSisley · 2023-02-13T17:03:17Z

question: Do you see any value in the new explain testing setup (which is still in the middle of migrating to the new setup) to adopt similar action / order based setup?

I have quite a strong preference to use this one framework for as much of our integration tests as possible. This includes explain, collection and P2P. If the code can be migrated in a way that does not notably complicate other actions.

Having all our tests work the same way should make things simpler (for us and externals), it also means we will be able to test more complex things (e.g. interleave P2P and other actions), and we'll get more for free by way of multiple stores and change detection.

shahzadlone · 2023-02-14T05:39:17Z

question: Do you see any value in the new explain testing setup (which is still in the middle of migrating to the new setup) to adopt similar action / order based setup?

I have quite a strong preference to use this one framework for as much of our integration tests as possible. This includes explain, collection and P2P. If the code can be migrated in a way that does not notably complicate other actions.

Having all our tests work the same way should make things simpler (for us and externals), it also means we will be able to test more complex things (e.g. interleave P2P and other actions), and we'll get more for free by way of multiple stores and change detection.

For reference when I say the new explain testing setup, I meant the changes introduced here: #949

Once all the old explain tests are converted to the new setup, I think we can either:

adjust the new explain test setup to be more similar to the actions like workflow in this framework,
OR adjust this framework to accommodate the new explain tests

For now I would strongly prefer that this new framework doesn't touch the explain tests (this PR doesn't), until they have been fully migrated to the new explain setup.

Other than that I don't mind trying this new setup and ironing out any kinks, I do like the explicit use of CollectionId but don't know how I feel about Actions: []any{} calling some testUtils.xyz operations (seems more complicated, and less explicit than before). But as this lets us control things we couldn't before I am willing to try this out :).

Curious to also hear other opinions on this new framework.

jsimnz · 2023-02-14T14:01:30Z

tests/integration/testCase.go

+	// Actions contains the set of actions and their expected results that
+	// this test should execute.  They will execute in the order that they
+	// are provided.
+	Actions []any


thought: Any thoughts to making this more type specific instead of any.

Such as defining a test case interface ie:

type TestCase interface {}

This gives us some future proofing options regarding how this can revolve. It can even potentially replace the switch case in executeTestCase and scope the actual action code to a method on the given test case.

Just a thought.

Yeah I started off with an empty interface/type expecting it to grow - it add no value though and so got removed. The switch could only be removed if introducing a general purpose cache/test-context object though, as some of the cases mutate stuff. I find the switch simpler than pushing everything into an interface with a somewhat forced func signature.

I was very much thinking of splitting up the testCase file, and locating the switch-cases within the respective file and this would lend itself well to that, I just dont think it is worth it atm and find the current simpler.

jsimnz

Please no camel casing for file names 🙃

fredcarle

I feel like this will be a good change as it will allow more intricate test configurations. Just a few things to discuss before approval.

fredcarle · 2023-02-14T04:30:16Z

tests/integration/testCase.go

+// using the collection api.
+type CreateDoc struct {
+	// The collection in which this document should be created.
+	CollectionId int


nitpick: I would encourage using idiomatic naming and if so Id should be ID.

I won't flag it everywhere but there is quite a few places where it's not idiomatic.

will change

ID

Don't forget DocId as well :)

😭 will do, forgot about that :)

DocID

TransactionId as well 🙈

fredcarle · 2023-02-14T15:32:00Z

tests/integration/changeDetector.go

@@ -0,0 +1,288 @@
+// Copyright 2022 Democratized Data Foundation


suggestion: Lets try to avoid camelcase file names :)

Also flagged by John - we do this in a few places I think, but will change

rename files

fredcarle · 2023-02-14T15:37:51Z

tests/integration/testCase.go

+	// Actions contains the set of actions and their expected results that
+	// this test should execute.  They will execute in the order that they
+	// are provided.
+	Actions []any


suggestion: Instead of any it might be worth having an interface type that has a function that executes the appropriate function for the action. This way we wouldn't need the switch case in the executeTestCase function and it would add a bit of type safety.

I see John has the same idea above. You can answer his and forget mine.

orpheuslummis

Cool PR. I prefer this new system to the previous. I wonder if testUtils "github.com/sourcenetwork/defradb/tests/integration" with a cleaner identifier, other than testUtils. I'm deferring LGTM only because I still don't have familiarity with our integration test suite to give a confident LGTM.

orpheuslummis · 2023-02-13T22:27:12Z

tests/integration/testCase.go

+//
+// A new transaction will be created for the first TransactionRequest2 of any given
+// TransactionId. TransactionRequest2s will be submitted to the database in the order
+// in which they are recieved (interleaving amoungst other actions if provided), however


typos: recieved, amoungst

cheers

typo

orpheuslummis · 2023-02-13T22:28:28Z

tests/integration/testCase.go

+
+	// If set to true, the request should yield no results and should instead timeout.
+	// The timeout is duration is that of subscriptionTimeout (1 second).
+	ExpectedTimout bool


typo: ExpectedTimout

cheers

typo

orpheuslummis · 2023-02-13T23:01:43Z

tests/integration/utils2.go

+var databaseDir string
+
+/*
+If this is set to true the integration test suite will instead of it's normal profile do


cheers

typo

orpheuslummis · 2023-02-15T01:55:50Z

tests/integration/utils2.go

+
+// Asserts as to whether an error has been raised as expected (or not). If an expected
+// error has been raised it will return true, returns false in all other cases.
+func AssertError(t *testing.T, description string, err error, expectedError string) bool {


idea: I'm not sure about this idea, but if this func would instead return an error on error, the branching as part of the switch of executeTestCase could read better. For example, instead of

if updateSchema(ctx, t, dbi.db, testCase, action) { return }

it could read more idiomatically like

if err := updateSchema(ctx, t, dbi.db, testCase, action); err != nil { return }

The error may be desired, and so this function will still need to exist in roughly its current form - it could be called outside the action loop in ExecuteTestCase and the errors returned from executeTestCase though perhaps.

Will have a look, could be a lot nicer

I had a look, whilst it sounds nice, in reality the executeTestCase function would need to return a list of errors, not just a single - this would result in other dirtiness as most lines can only yield singles and they would need to wrap their error into a slice. Can be revisited later ofc, but for now I think it is best as is.

Thanks for considering it!

I think this is getting changed slightly in the next few min - I forgot to assert that the expected errors were actually raised, and on revisit I am not even sure that we want the early return in the new test framework as expected errors are scoped to a specific action (and we may want other actions to be done after them, e.g. to assert transaction rollback/not-applied)

orpheuslummis · 2023-02-15T01:57:47Z

tests/integration/utils2.go

+	return startIndex, endIndex
+}
+
+// getCollections returns all the collections of the given name, preserving order.


suggestion: "name" -> "names"

cheers

reword

orpheuslummis · 2023-02-15T02:03:35Z

tests/integration/utils2.go

+) []client.Collection {
+	collections := make([]client.Collection, len(collectionNames))
+
+	allCollections, err := db.GetAllCollections(ctx)


question(non-blocking): would leveraging db.GetCollectionByName instead make it more efficient?

GetCollectionByName errors if the collection is not found and that is undesirable at this point. Im also not sure it would be more performant in most cases (as typically this will yield all the collections anyway)

orpheuslummis · 2023-02-15T02:04:18Z

tests/integration/utils2.go

+	return AssertError(t, testCase.Description, err, action.ExpectedError)
+}
+
+// createDoc creates a document using the collection api and caches it in the


nitpick: personal preference is to use "API" instead of "api"

If I find myself in the area I'll maybe change this 😆

orpheuslummis · 2023-02-15T02:17:45Z

tests/integration/utils2.go

+	}
+
+	result := db.ExecTransactionalRequest(ctx, action.Request, txns[action.TransactionId])
+	done := assertRequestResults(


idea(nonblocking, uncertain): rename to erroneous or failure or so, as it reads better.

you mean rename done? If so erroneous or particularly failure would be incorrect. if done is true it means that the expected error has been found (and no others) and the test should immediately pass. Any unexpected errors just get asserted like normal, in which case done does not matter.

orpheuslummis · 2023-02-15T02:19:48Z

tests/integration/utils2.go

+}
+
+// executeSubscriptionRequest executes the given subscription request, returning
+// a channel that will recieve a single event once the subscription has been completed.


typo: recieve

orpheuslummis · 2023-02-15T02:22:38Z

tests/integration/testCase.go

+	Request string
+
+	// If set to true, the request should yield no results and should instead timeout.
+	// The timeout is duration is that of subscriptionTimeout (1 second).


nitpick: remove the "1 second" comment for future clarity?

😁 I wanted to do that as I felt the same way, comment predates this PR and was just moved, I felt it might be impolite to change lol (trading someone else's personal preference for my own).

Will drop it now as you have flagged it too (and extra is)

I think someone had asked to have the "1 second" there in the PR that created it. I don't really care if it's there or not.

Maybe remove the extra "is" while you're there though lol.

It is only half broken, in that this update has no effect on the production code under test, but the incorrect index causes a panic in the new system, whereas the old one silently ignored the value.

Creates a new integration test framework, and migrates shared functions to its files from the old framework. The Init code is unchanged, as is the change detector code - they have just moved. Is not in use until the next commit - I thought it might be easier to review if split into two. It provides the same features as the old system, but without implicit/ridged order of operations, and hopefully simpler code. Bonus fix - transactional tests now work with the change detector.

Changes the old test framework so that it is essentially syntax sugar on top of the new framework. This is temporary, and is done to allow us to introduce (and review) the new framework without having to migrate all our existing tests to the new framework. The old framework should be removed as soon as is convienent, and can be done so bit by bit. It also ensures that the new framework contains all the functionality of the old framework. Explain and collection tests use a different framework. Those frameworks have not been migrated here, but their tests should be migrated at some point - it should be fairly straight forward to extend the new test framework to do so. New integration tests should be written using the new framework only. This includes collection tests and explain tests.

Are numbers upper case?

Sorry about the late change, there was a bug in the subscription mapping from the old system that resulted in these silently passing. It proved inconvienent to fix in the mapping as the old system did not specify the results for the triggering mutations which are required in the new system. I decided it would be better to just convert the existing tests to the new system.

Also converts one of the transaction tests, as the old system does not specify which action should expect an error, just that one action should raise it. That is not particularly great in my opinion and this commit changes the new system so that as well as tying the error to a particular action (as it did before), actions declared after an action with an expected action will still be executed - the old system would instead exit early, the request in the old version of the converted test was never actually executed.

codecov · 2023-02-15T22:58:44Z

Codecov Report

Merging #1089 (290d0a2) into develop (ac128af) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1089      +/-   ##
===========================================
- Coverage    67.53%   67.51%   -0.02%     
===========================================
  Files          181      181              
  Lines        16551    16551              
===========================================
- Hits         11178    11175       -3     
- Misses        4435     4437       +2     
- Partials       938      939       +1

Impacted Files	Coverage Δ
datastore/badger/v3/datastore.go	`38.63% <0.00%> (-0.49%)`	⬇️

AndrewSisley · 2023-02-15T23:16:52Z

I wonder if testUtils "github.com/sourcenetwork/defradb/tests/integration" with a cleaner identifier

Shahzad also mentioned this. I consider this to be independent of this PR - that alias can be changed regardless of if/when this PR gets merged, and modifying the test-containing files themselves is explicitly out of scope of this PR.

I personally dont mind testUtils.Foo, but I dont mind it changing either. I definately don't want to change all our test files in this PR though.

fredcarle

LGTM. I like this change and I look forward to using it :)

shahzadlone · 2023-02-16T07:20:19Z

I wonder if testUtils "github.com/sourcenetwork/defradb/tests/integration" with a cleaner identifier

Shahzad also mentioned this. I consider this to be independent of this PR - that alias can be changed regardless of if/when this PR gets merged, and modifying the test-containing files themselves is explicitly out of scope of this PR.

I personally dont mind testUtils.Foo, but I dont mind it changing either. I definately don't want to change all our test files in this PR though.

Agreed that it's best to keep those outside this PRs scope.

Requested changes have been made

* Fix broken test It is only half broken, in that this update has no effect on the production code under test, but the incorrect index causes a panic in the new system, whereas the old one silently ignored the value. * Create new integration test framework Creates a new integration test framework, and migrates shared functions to its files from the old framework. The Init code is unchanged, as is the change detector code - they have just moved. Is not in use until the next commit - I thought it might be easier to review if split into two. It provides the same features as the old system, but without implicit/ridged order of operations, and hopefully simpler code. Bonus fix - transactional tests now work with the change detector. * Make old test framework to use new framework Changes the old test framework so that it is essentially syntax sugar on top of the new framework. This is temporary, and is done to allow us to introduce (and review) the new framework without having to migrate all our existing tests to the new framework. The old framework should be removed as soon as is convienent, and can be done so bit by bit. It also ensures that the new framework contains all the functionality of the old framework. Explain and collection tests use a different framework. Those frameworks have not been migrated here, but their tests should be migrated at some point - it should be fairly straight forward to extend the new test framework to do so. New integration tests should be written using the new framework only. This includes collection tests and explain tests.

AndrewSisley added area/testing Related to any test or testing suite action/no-benchmark Skips the action that runs the benchmark. labels Feb 9, 2023

AndrewSisley added this to the DefraDB v0.5 milestone Feb 9, 2023

AndrewSisley requested a review from a team February 9, 2023 18:48

AndrewSisley self-assigned this Feb 9, 2023

AndrewSisley force-pushed the sisley/refactor/I1087-int-test-rework branch 4 times, most recently from 48238de to b75b223 Compare February 9, 2023 20:14

shahzadlone reviewed Feb 12, 2023

View reviewed changes

tests/integration/explain/default/delete_test.go Show resolved Hide resolved

AndrewSisley requested a review from shahzadlone February 13, 2023 17:03

jsimnz reviewed Feb 14, 2023

View reviewed changes

jsimnz previously requested changes Feb 14, 2023

View reviewed changes

fredcarle reviewed Feb 14, 2023

View reviewed changes

orpheuslummis reviewed Feb 15, 2023

View reviewed changes

AndrewSisley force-pushed the sisley/refactor/I1087-int-test-rework branch 11 times, most recently from b8cd2a0 to 6bf5e9a Compare February 15, 2023 20:07

AndrewSisley requested a review from jsimnz February 15, 2023 20:13

AndrewSisley force-pushed the sisley/refactor/I1087-int-test-rework branch 5 times, most recently from da43279 to 2ea5f10 Compare February 15, 2023 22:51

AndrewSisley added 11 commits February 15, 2023 17:52

Fix broken test

4ec5108

It is only half broken, in that this update has no effect on the production code under test, but the incorrect index causes a panic in the new system, whereas the old one silently ignored the value.

PR FIXUP - ID not Id

458d3c6

PR FIXUP - Fix spellings

839e25a

PR FIXUP - Drop variable value from comment

aefa8be

PR FIXUP - DocID not DocId

1632631

PR FIXUP - Remove file camel casing

50eb0bd

Are numbers upper case?

PR FIXUP - Drop internal subscription struct

2ea5f10

AndrewSisley force-pushed the sisley/refactor/I1087-int-test-rework branch from 274b694 to 290d0a2 Compare February 15, 2023 23:04

PR FIXUP - TransactionID not TransactionId

290d0a2

fredcarle approved these changes Feb 16, 2023

View reviewed changes

shahzadlone approved these changes Feb 16, 2023

View reviewed changes

AndrewSisley merged commit 6485f82 into develop Feb 16, 2023

AndrewSisley deleted the sisley/refactor/I1087-int-test-rework branch February 16, 2023 16:44

shahzadlone mentioned this pull request Mar 28, 2023

Integrate the new explain test setup into the new test action system #1243

Closed

shahzadlone mentioned this pull request Apr 20, 2023

Convert the explain tests to use the new explain setup #953

Closed

24 tasks

		@@ -0,0 +1,288 @@
		// Copyright 2022 Democratized Data Foundation

refactor: Rework integration test framework #1089

refactor: Rework integration test framework #1089

Conversation

AndrewSisley commented Feb 9, 2023 • edited Loading

Relevant issue(s)

Description

shahzadlone commented Feb 12, 2023

AndrewSisley commented Feb 13, 2023

shahzadlone commented Feb 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orpheuslummis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 15, 2023 • edited Loading

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

AndrewSisley Feb 15, 2023 • edited Loading

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

AndrewSisley Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 15, 2023 • edited Loading

Codecov Report

AndrewSisley commented Feb 15, 2023

fredcarle left a comment

Choose a reason for hiding this comment

shahzadlone commented Feb 16, 2023

AndrewSisley commented Feb 9, 2023 •

edited

Loading

AndrewSisley Feb 15, 2023 •

edited

Loading

AndrewSisley Feb 15, 2023 •

edited

Loading

AndrewSisley Feb 15, 2023 •

edited

Loading

AndrewSisley Feb 15, 2023 •

edited

Loading

AndrewSisley Feb 15, 2023 •

edited

Loading

AndrewSisley Feb 15, 2023 •

edited

Loading

AndrewSisley Feb 15, 2023 •

edited

Loading

AndrewSisley Feb 15, 2023 •

edited

Loading

AndrewSisley Feb 15, 2023 •

edited

Loading

AndrewSisley Feb 15, 2023 •

edited

Loading

codecov bot commented Feb 15, 2023 •

edited

Loading