ci: Parallelize change detector #1871

nasdf · 2023-09-12T18:49:38Z

Relevant issue(s)

Resolves #1436

Description

This PR allows the change detector to run in parallel with other tests.

There are also a few other improvements to the change detector:

change detector logic is moved to a new tests/change_detector package
- change detector specific logic in tests/integration is now more obvious
change detector now operates on distinct source and target branches
- this enables you to test branches outside of the currently checked out branch
change detector manages its own environment variables.
- running go test ./tests/change_detector/... will work without env variables
change detector temp directories are now all cleaned up after running

Todo before merging:

update default branch and repository
document change detector data format changes

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make test:changes

Specify the platform(s) on which this was tested:

MacOS

…ts in parallel

… init logic. add data format documentation checks

…nted and getDatabaseFormatDocumentation

codecov · 2023-09-12T19:06:28Z

Codecov Report

Patch coverage has no change and project coverage change: -0.01% ⚠️

Comparison is base (2020a1e) 70.31% compared to head (cf6267f) 70.30%.

@@             Coverage Diff             @@
##           develop    #1871      +/-   ##
===========================================
- Coverage    70.31%   70.30%   -0.01%     
===========================================
  Files          232      232              
  Lines        24192    24192              
===========================================
- Hits         17010    17007       -3     
- Misses        6021     6023       +2     
- Partials      1161     1162       +1

Flag	Coverage Δ
all-tests	`70.30% <ø> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

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

AndrewSisley · 2023-09-13T18:34:51Z

tests/change_detector/change_detector_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestChanges(t *testing.T) {


praise: I really like this approach, it is simpler and neater than some kind of file lock IMO.

AndrewSisley · 2023-09-13T18:40:21Z

tests/change_detector/change_detector_test.go

+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package change_detector


suggestion(non-blocking, minor): Whilst I like the approach, I do dislike that this test will be executed by default with go test ./..., I think it might be nicer if we exclude this test by default, perhaps using a build flag. This would however complicate the github action matrix stuff.

Added build tag here and updated Makefile to include --tags change_detector.

AndrewSisley · 2023-09-13T18:41:41Z

Makefile

@@ -294,8 +295,7 @@ test\:coverage-html:

 .PHONY: test\:changes
 test\:changes:
-	@$(MAKE) deps:lens
-	env DEFRA_DETECT_DATABASE_CHANGES=true DEFRA_CLIENT_GO=true gotestsum -- ./... -shuffle=on -p 1
+	gotestsum --format testname -- ./$(CHANGE_DETECTOR_TEST_DIRECTORY)/... -shuffle=on


nitpick: It does look like -shuffle=on adds no value here? Or is it passed down into the child tests?

Removed shuffle flag.

shahzadlone · 2023-09-14T11:47:43Z

tests/integration/utils2.go

@@ -202,7 +155,7 @@ func init() {
 //
 //	Usage: AssertPanicAndSkipChangeDetection(t, func() { executeTestCase(t, test) })
 func AssertPanicAndSkipChangeDetection(t *testing.T, f assert.PanicTestFunc) bool {
-	if IsDetectingDbChanges() {
+	if change_detector.Enabled {


nitpick: Perhaps a non-snake case package name or import alias name.

Updated to changeDetector with an alias.

shahzadlone

I do really like these changes, specially the clean isolation of change detection test under the test/ folder and ability to specify source and target in a clean manner.

…akefile target

nasdf · 2023-09-18T19:03:08Z

I do really like these changes, specially the clean isolation of change detection test under the test/ folder and ability to specify source and target in a clean manner.

I've updated it to default to the local repo if target is not defined. This should make it simpler to run in CI.

AndrewSisley · 2023-09-18T19:09:47Z

tests/change_detector/change_detector_test.go

+
+	var targetRepoDir string
+	if targetBranch == "" {
+		// default to the local branch


question: This seems odd and a bit dangerous no? If I understand correctly this will now default to testing against itself, which is essentially the same as running go test ./...?

This does seem like an easy way to accidentally not run the change detector at all.

EDIT: I misunderstood, we now have source and target branches, and the new source branch is what the old target used to be. Ignore this comment :)

…ream defradb repo. update checkIfDatabaseFormatChangesAreDocumented to use source and target directories

shahzadlone

Question: were you able to introduce breaking changes locally and test that the change detector actually catches the breaking change?

nasdf · 2023-09-18T21:52:54Z

Question: were you able to introduce breaking changes locally and test that the change detector actually catches the breaking change?

No, but I can do that and report back.

Update: I was able to get it to fail, but it's also failing on this test case which should be passing.

--- FAIL: TestMutationUpdateOneToMany_InvalidAliasRelationNameToLinkFromManySide_Collection (0.02s)
        	            	panic: runtime error: index out of range [0] with length 0 [recovered]
        	            		panic: runtime error: index out of range [0] with length 0
        	            	
        	            	goroutine 1926 [running]:
        	            	testing.tRunner.func1.2({0x1059a91c0, 0x14000518048})
        	            		/usr/local/go/src/testing/testing.go:1526 +0x1c8
        	            	testing.tRunner.func1()
        	            		/usr/local/go/src/testing/testing.go:1529 +0x384
        	            	panic({0x1059a91c0, 0x14000518048})
        	            		/usr/local/go/src/runtime/panic.go:884 +0x204
        	            	github.com/sourcenetwork/defradb/tests/integration.updateDocViaColSave(0x140008352d8?, {{0x0, 0x0}, 0x0, 0x0, {0x14000041a40, 0x3d}, {0x105395e96, 0x2c}, 0x0}, ...)
        	            		/Users/keenan/Projects/defradb/tests/integration/utils2.go:1204 +0x110
        	            	github.com/sourcenetwork/defradb/tests/integration.updateDoc.func1()
        	            		/Users/keenan/Projects/defradb/tests/integration/utils2.go:1190 +0x80
        	            	github.com/sourcenetwork/defradb/tests/integration.withRetry({0x14000010068, 0x1, 0x140008353b8?}, 0x0, 0x14000835440)
        	            		/Users/keenan/Projects/defradb/tests/integration/utils2.go:1403 +0x90
        	            	github.com/sourcenetwork/defradb/tests/integration.updateDoc(0x14000a37a00, {{0x0, 0x0}, 0x0, 0x0, {0x14000041a40, 0x3d}, {0x105395e96, 0x2c}, 0x0})
        	            		/Users/keenan/Projects/defradb/tests/integration/utils2.go:1187 +0x2e0
        	            	github.com/sourcenetwork/defradb/tests/integration.executeTestCase({0x105aa7670?, 0x14000196018}, 0x14000a37860, {0x140004ba080, 0x2, 0x2}, {{0x1053b3d45, 0x44}, {0x1400052d680, 0x4, ...}, ...}, ...)
        	            		/Users/keenan/Projects/defradb/tests/integration/utils2.go:385 +0xf40
        	            	github.com/sourcenetwork/defradb/tests/integration.ExecuteTestCase(0x140004cfd50?, {{0x1053b3d45, 0x44}, {0x1400052d680, 0x4, 0x4}, {0x1, {0x140004ba060, 0x2, 0x2}}})
        	            		/Users/keenan/Projects/defradb/tests/integration/utils2.go:311 +0x504
        	            	github.com/sourcenetwork/defradb/tests/integration/mutation/update/field_kinds/one_to_many.executeTestCase(...)
        	            		/Users/keenan/Projects/defradb/tests/integration/mutation/update/field_kinds/one_to_many/utils.go:20
        	            	github.com/sourcenetwork/defradb/tests/integration/mutation/update/field_kinds/one_to_many.TestMutationUpdateOneToMany_InvalidAliasRelationNameToLinkFromManySide_Collection(0x1400015f230?)
        	            		/Users/keenan/Projects/defradb/tests/integration/mutation/update/field_kinds/one_to_many/with_alias_test.go:255 +0x458
        	            	testing.tRunner(0x14000a37860, 0x105a8c758)
        	            		/usr/local/go/src/testing/testing.go:1576 +0x10c
        	            	created by testing.(*T).Run
        	            		/usr/local/go/src/testing/testing.go:1629 +0x368
        	            	FAIL	github.com/sourcenetwork/defradb/tests/integration/mutation/update/field_kinds/one_to_many	1.769s
        	            	FAIL

nasdf · 2023-09-19T19:10:50Z

Question: were you able to introduce breaking changes locally and test that the change detector actually catches the breaking change?

No, but I can do that and report back.

Update: I was able to get it to fail, but it's also failing on this test case which should be passing.

--- FAIL: TestMutationUpdateOneToMany_InvalidAliasRelationNameToLinkFromManySide_Collection (0.02s)
        	            	panic: runtime error: index out of range [0] with length 0 [recovered]
        	            		panic: runtime error: index out of range [0] with length 0
        	            	
        	            	goroutine 1926 [running]:
        	            	testing.tRunner.func1.2({0x1059a91c0, 0x14000518048})
        	            		/usr/local/go/src/testing/testing.go:1526 +0x1c8
        	            	testing.tRunner.func1()
        	            		/usr/local/go/src/testing/testing.go:1529 +0x384
        	            	panic({0x1059a91c0, 0x14000518048})
        	            		/usr/local/go/src/runtime/panic.go:884 +0x204
        	            	github.com/sourcenetwork/defradb/tests/integration.updateDocViaColSave(0x140008352d8?, {{0x0, 0x0}, 0x0, 0x0, {0x14000041a40, 0x3d}, {0x105395e96, 0x2c}, 0x0}, ...)
        	            		/Users/keenan/Projects/defradb/tests/integration/utils2.go:1204 +0x110
        	            	github.com/sourcenetwork/defradb/tests/integration.updateDoc.func1()
        	            		/Users/keenan/Projects/defradb/tests/integration/utils2.go:1190 +0x80
        	            	github.com/sourcenetwork/defradb/tests/integration.withRetry({0x14000010068, 0x1, 0x140008353b8?}, 0x0, 0x14000835440)
        	            		/Users/keenan/Projects/defradb/tests/integration/utils2.go:1403 +0x90
        	            	github.com/sourcenetwork/defradb/tests/integration.updateDoc(0x14000a37a00, {{0x0, 0x0}, 0x0, 0x0, {0x14000041a40, 0x3d}, {0x105395e96, 0x2c}, 0x0})
        	            		/Users/keenan/Projects/defradb/tests/integration/utils2.go:1187 +0x2e0
        	            	github.com/sourcenetwork/defradb/tests/integration.executeTestCase({0x105aa7670?, 0x14000196018}, 0x14000a37860, {0x140004ba080, 0x2, 0x2}, {{0x1053b3d45, 0x44}, {0x1400052d680, 0x4, ...}, ...}, ...)
        	            		/Users/keenan/Projects/defradb/tests/integration/utils2.go:385 +0xf40
        	            	github.com/sourcenetwork/defradb/tests/integration.ExecuteTestCase(0x140004cfd50?, {{0x1053b3d45, 0x44}, {0x1400052d680, 0x4, 0x4}, {0x1, {0x140004ba060, 0x2, 0x2}}})
        	            		/Users/keenan/Projects/defradb/tests/integration/utils2.go:311 +0x504
        	            	github.com/sourcenetwork/defradb/tests/integration/mutation/update/field_kinds/one_to_many.executeTestCase(...)
        	            		/Users/keenan/Projects/defradb/tests/integration/mutation/update/field_kinds/one_to_many/utils.go:20
        	            	github.com/sourcenetwork/defradb/tests/integration/mutation/update/field_kinds/one_to_many.TestMutationUpdateOneToMany_InvalidAliasRelationNameToLinkFromManySide_Collection(0x1400015f230?)
        	            		/Users/keenan/Projects/defradb/tests/integration/mutation/update/field_kinds/one_to_many/with_alias_test.go:255 +0x458
        	            	testing.tRunner(0x14000a37860, 0x105a8c758)
        	            		/usr/local/go/src/testing/testing.go:1576 +0x10c
        	            	created by testing.(*T).Run
        	            		/usr/local/go/src/testing/testing.go:1629 +0x368
        	            	FAIL	github.com/sourcenetwork/defradb/tests/integration/mutation/update/field_kinds/one_to_many	1.769s
        	            	FAIL

After some further digging I've verified that this test is not correctly persisting the document from the setup phase.

https://github.com/sourcenetwork/defradb/blob/develop/tests/integration/mutation/update/field_kinds/one_to_many/with_alias_test.go#L231-L240

If this is a blocker for merging I can look into fixing it in a new PR.

shahzadlone · 2023-09-19T20:36:56Z

I don't see that as a blocker as long as change detection works. Going to review one last time and give an LGTM in a few minutes

nasdf · 2023-09-19T20:47:33Z

I don't see that as a blocker as long as change detection works. Going to review one last time and give an LGTM in a few minutes

For clarification #1897 fixed this failing test.

shahzadlone

LGTM

shahzadlone · 2023-09-19T21:16:02Z

nitpick: Slight preference for the PR title to be a ci label, with first word as an action word. i.e. ci: Parallelize change detector

## Relevant issue(s) Closes #1472 Closes #1507 Closes #1860 ## Description This is a follow up to #1776 This PR adds a CLI implementation that implements the client.DB interface and runs through the existing integration test suite. - [x] Merge existing server config code - [x] Refactor CLI to use new HTTP client - [x] Remove `net/api` package - [x] Remove `api/http` package - [x] Lens tests are timing out in CI: fixed #1862 - [x] Code coverage is incorrectly reporting: fixed #1861 - [x] Flaky test causing failures: fixed #1912 Renamed Commands: - `peerid` to `client peer info` - `client p2pcollection` to `client p2p collection` - `client replicator` to `client p2p replicator` - `client schema list` to `client collection describe` Removed Commands: - `block get` - `ping` - `rpc` Added Commands: - `client collection create` - `client collection delete` - `client collection get` - `client collection keys` - `client collection update` - `client tx create` - `client tx discard` - `client tx commit` - `client schema migration up` - `client schema migration down` - `client schema migration reload` **Notes for reviewers**: - `.github` changes are merged from #1871 - `Makefile` most of these changes are also from #1871 - `docs/cli` ignore these changes, it will be updated next release - sorry for all of the merge commits, I am working on learning rebase flow ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... ## How has this been tested? `make test` Specify the platform(s) on which this was tested: - MacOS

## Relevant issue(s) Resolves sourcenetwork#1436 ## Description This PR allows the change detector to run in parallel with other tests. There are also a few other improvements to the change detector: - change detector logic is moved to a new `tests/change_detector` package - change detector specific logic in `tests/integration` is now more obvious - change detector now operates on distinct `source` and `target` branches - this enables you to test branches outside of the currently checked out branch - change detector manages its own environment variables. - running `go test ./tests/change_detector/...` will work without env variables - change detector temp directories are now all cleaned up after running Todo before merging: - [x] update default branch and repository - [x] document change detector data format changes ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... ## How has this been tested? `make test:changes` Specify the platform(s) on which this was tested: - MacOS

## Relevant issue(s) Closes sourcenetwork#1472 Closes sourcenetwork#1507 Closes sourcenetwork#1860 ## Description This is a follow up to sourcenetwork#1776 This PR adds a CLI implementation that implements the client.DB interface and runs through the existing integration test suite. - [x] Merge existing server config code - [x] Refactor CLI to use new HTTP client - [x] Remove `net/api` package - [x] Remove `api/http` package - [x] Lens tests are timing out in CI: fixed sourcenetwork#1862 - [x] Code coverage is incorrectly reporting: fixed sourcenetwork#1861 - [x] Flaky test causing failures: fixed sourcenetwork#1912 Renamed Commands: - `peerid` to `client peer info` - `client p2pcollection` to `client p2p collection` - `client replicator` to `client p2p replicator` - `client schema list` to `client collection describe` Removed Commands: - `block get` - `ping` - `rpc` Added Commands: - `client collection create` - `client collection delete` - `client collection get` - `client collection keys` - `client collection update` - `client tx create` - `client tx discard` - `client tx commit` - `client schema migration up` - `client schema migration down` - `client schema migration reload` **Notes for reviewers**: - `.github` changes are merged from sourcenetwork#1871 - `Makefile` most of these changes are also from sourcenetwork#1871 - `docs/cli` ignore these changes, it will be updated next release - sorry for all of the merge commits, I am working on learning rebase flow ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... ## How has this been tested? `make test` Specify the platform(s) on which this was tested: - MacOS

nasdf added 12 commits September 11, 2023 21:52

move change detector logic to tests/change_detector so it can run tes…

605f428

…ts in parallel

set rootDatabaseDir from env variable

de1a1eb

set databaseDir instead of rootDatabaseDir

5b389cb

cleanup database file path unused variables

f6bb25b

set count flag in go test command

dc5aaf8

always set detect changes env

6ab7371

fix bug in badger file path

8195242

restore preflight checks and db path logic

01afa55

enable parallel change detector

4b9c36e

add more documentation

9ec616a

update Makefile test targets

3b84d9f

move all change detector logic to new package. clean up env names and…

536f6fe

… init logic. add data format documentation checks

nasdf added the area/testing Related to any test or testing suite label Sep 12, 2023

nasdf added this to the DefraDB v0.8 milestone Sep 12, 2023

nasdf self-assigned this Sep 12, 2023

nasdf added 2 commits September 12, 2023 11:59

replace panic with test.Fail in checkIfDatabaseFormatChangesAreDocume…

b312303

…nted and getDatabaseFormatDocumentation

update test:changes Makefile target test flags

6039650

nasdf mentioned this pull request Sep 12, 2023

ci: Test matrix #1862

Closed

8 tasks

AndrewSisley reviewed Sep 13, 2023

View reviewed changes

shahzadlone reviewed Sep 14, 2023

View reviewed changes

nasdf added 2 commits September 18, 2023 11:30

add change_detector build flag. remove shuffle from change_detector M…

f6af08b

…akefile target

change detector target defaults to local repo

eb80d80

AndrewSisley reviewed Sep 18, 2023

View reviewed changes

nasdf added 2 commits September 18, 2023 12:24

document change detector changes. set default branch and repo to upst…

97396de

…ream defradb repo. update checkIfDatabaseFormatChangesAreDocumented to use source and target directories

Merge branch 'develop' into nasdf/test/parallel-change-detector

08a1186

nasdf and others added 3 commits September 18, 2023 12:33

fix linter errors

fc6cbcd

fix build errors

cfc1182

Merge branch 'develop' into nasdf/test/parallel-change-detector

de3e575

nasdf marked this pull request as ready for review September 18, 2023 21:01

nasdf requested a review from a team September 18, 2023 21:02

shahzadlone reviewed Sep 18, 2023

View reviewed changes

nasdf added 2 commits September 19, 2023 10:50

add config logging

b01d301

move test logging to executeTestCase

3626037

shahzadlone approved these changes Sep 19, 2023

View reviewed changes

nasdf changed the title ~~test: Parallel change detector~~ ci: Parallelize change detector Sep 19, 2023

Merge branch 'develop' into nasdf/test/parallel-change-detector

cf6267f

nasdf merged commit b8567c2 into sourcenetwork:develop Sep 19, 2023
14 checks passed

nasdf deleted the nasdf/test/parallel-change-detector branch September 19, 2023 21:51

nasdf mentioned this pull request Sep 27, 2023

refactor: CLI client interface #1839

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: Parallelize change detector #1871

ci: Parallelize change detector #1871

nasdf commented Sep 12, 2023 •

edited

Loading

codecov bot commented Sep 12, 2023 •

edited

Loading

AndrewSisley Sep 13, 2023

AndrewSisley Sep 13, 2023

nasdf Sep 18, 2023

AndrewSisley Sep 13, 2023

nasdf Sep 18, 2023

shahzadlone Sep 14, 2023

nasdf Sep 18, 2023

shahzadlone left a comment

nasdf commented Sep 18, 2023

AndrewSisley Sep 18, 2023 •

edited

Loading

shahzadlone left a comment

nasdf commented Sep 18, 2023 •

edited

Loading

nasdf commented Sep 19, 2023

shahzadlone commented Sep 19, 2023

nasdf commented Sep 19, 2023

shahzadlone left a comment

shahzadlone commented Sep 19, 2023

ci: Parallelize change detector #1871

ci: Parallelize change detector #1871

Conversation

nasdf commented Sep 12, 2023 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Sep 12, 2023 • edited Loading

Codecov Report

AndrewSisley Sep 13, 2023

Choose a reason for hiding this comment

AndrewSisley Sep 13, 2023

Choose a reason for hiding this comment

nasdf Sep 18, 2023

Choose a reason for hiding this comment

AndrewSisley Sep 13, 2023

Choose a reason for hiding this comment

nasdf Sep 18, 2023

Choose a reason for hiding this comment

shahzadlone Sep 14, 2023

Choose a reason for hiding this comment

nasdf Sep 18, 2023

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

nasdf commented Sep 18, 2023

AndrewSisley Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

nasdf commented Sep 18, 2023 • edited Loading

nasdf commented Sep 19, 2023

shahzadlone commented Sep 19, 2023

nasdf commented Sep 19, 2023

shahzadlone left a comment

Choose a reason for hiding this comment

shahzadlone commented Sep 19, 2023

nasdf commented Sep 12, 2023 •

edited

Loading

codecov bot commented Sep 12, 2023 •

edited

Loading

AndrewSisley Sep 18, 2023 •

edited

Loading

nasdf commented Sep 18, 2023 •

edited

Loading