test: Add example CLI integration tests #3550

AndrewSisley · 2025-03-19T16:27:13Z

Relevant issue(s)

Resolves #3569

Description

Adds the example CLI integration tests using new test framework.

Largely as seen in the demo last week, but with a little more polish. I think it is good to commit these now and then we can expand them as/when we feel like, and use it as an example for other user-facing stuff (like http).

I have not added the txn-commit multiplier to the CI, as the workflow test files are fairly coupled to executing all tests in the entire repository and I wanted to limit scope here.

Tasks

The testing repo dependency is currently a local, relative, path and needs to be pointed at https://github.com/sourcenetwork/testing when feat: Initial version testo#1 gets merged.
Link this to a proper issue if/when feat: Initial version testo#1 is merged

## Relevant issue(s) Resolves #3551 ## Description Only give badger a path if not in-memory. Credit for fix belongs to Keenan not me. I broke this bringing corekv in. Keenan fixed this, I'm just doing the typing. This is tested in #3550

islamaliev

Look very nice, Andy! I'm happy with the changes in general. Have some suggestions and questions.

islamaliev · 2025-03-24T19:32:47Z

cli/test/action/action.go

+//
+// It is typically used by multipliers in order to provide additional args to Actions
+// implementing this interface.
+type Argmented interface {


question: is it a word? Didn't you want to write "Augmented"?

I guess I did 😁

Use correct spelling

islamaliev · 2025-03-24T20:08:43Z

cli/test/action/action.go

+func execute(ctx context.Context, args []string) error {
+	stdOut, stdErr, err := executeStream(ctx, args)
+	if err != nil {
+		return err
+	}
+	_, err = io.ReadAll(stdOut)
+	if err != nil {
+		return err
+	}
+	stdErrData, err := io.ReadAll(stdErr)
+	if err != nil {
+		return err
+	}
+	if len(stdErrData) != 0 {
+		return fmt.Errorf("%s", stdErrData)
+	}
+	return nil
+}


suggestion: this block is almost identical to the executeBytes. I would strongly suggest wrap it:

func execute(ctx context.Context, args []string) error { _, err := executeBytes(ctx, args) return err }

Ah of course, thanks Islam, is a silly mistake of mine :)

Cleanup execute

islamaliev · 2025-03-24T20:40:28Z

cli/test/action/collection_describe.go

+	result, err := executeJson[[]client.CollectionDefinition](a.s.Ctx, args)
+	require.NoError(a.s.T, err)
+
+	require.Equal(a.s.T, len(a.Expected), len(result))


note: these assets won't make it easy to spot the failing action (if there are several, like create doc).
Would be nice add something like CodeLocation in ginkgo. But it's probably for a later ticket.

I agree, we use something similar in the main integration test framework (assertStack), I'd like to use that here but didn't want to spend the time right now.

islamaliev · 2025-03-24T20:45:27Z

cli/test/action/start.go

+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(a.s.T, err)
+
+	args := []string{"start", "--no-keyring", "--store=memory"}


question: why are these args hard-coded? What if want to play around with keyring cli tests?

Just a temporary thing, I didn't want to setup whatever would be needed (multipliers) to allow these to change without code changes.

islamaliev · 2025-03-24T21:13:17Z

cli/test/multiplier/txn.go

+			if argumentedAction, ok := a.(action.ArgmentedAction); ok && i > lastStartIndex {
+				result = append(result, action.WithTxn(argumentedAction))
+			} else {
+				result = append(result, a)
+			}


todo: I believe we should document this not quite explicit behaviour: wrapping only action.ArgmentedAction-compliant actions with transaction and skipping the rest.
And also the fact that the transaction is being created after Start command and commit is added to the end. Would be nice to have a general comment for the multiplier that mentions all that.

I thought I added documentation, but I guess I only intended too... Thanks for flagging Islam :)

Document txn multiplier

islamaliev · 2025-03-24T21:14:29Z

cli/test/multiplier/txn.go

+
+		switch a.(type) {
+		// Append orginal, read-only actions that occured after the last write action,
+		// after the transaction has been commited.  This allows the automatic of whether


nitpick: I think a work is missing after "automatic"

yes :) Thanks

Add missing word(s?)

islamaliev · 2025-03-24T21:17:04Z

cli/test/multiplier/txn.go

+		case *action.SchemaAdd:
+			lastWriteIndex = i


suggestion: this part, I guess, check if the current action is read-only or not and the list will grow when we add more write actions, right? Would be nice to have a comment here mentioning that.
Better yet, to have a ticket about some proper handling of read-only action and linking it here.

Will add

Document lastWriteIndex

This documentation has been added to the multiplier-level documentation, not the internal implementation/variable

islamaliev · 2025-03-24T21:19:24Z

cli/test/multiplier/txn.go

+		switch a.(type) {
+		// Append orginal, read-only actions that occured after the last write action,
+		// after the transaction has been commited.  This allows the automatic of whether
+		// or not the transaction-state has been persisted.


suggestion: very helpful comment. Please include this also in the documentation for the multiplier.

islamaliev · 2025-03-24T21:24:34Z

cli/test/state/state.go

+
+type State struct {
+	Ctx     context.Context
+	Cancels []context.CancelFunc


suggestion: this term feel a bit unnatural for the testing. I think it would be better to all is something like Cleanup or TearDown. And maybe it can be of type []func() to make it more generic.
Anyway a comment also would be nice here.

question: does it need to be public? Maybe it's would be better to add a public append-only method?

does it need to be public? Maybe it's would be better to add a public append-only method?

At the moment it only really gets added here because the linter demands that the timeout cancel is referenced, and while I was building this at somepoint it was used.

I think I'll remove it and add a //nolint instead

Remove Cancels

codecov · 2025-03-25T16:56:51Z

Codecov Report

Attention: Patch coverage is 55.89226% with 131 lines in your changes missing coverage. Please review.

Project coverage is 78.64%. Comparing base (acb43fa) to head (16c02c4).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
cli/test/action/action.go	64.15%	27 Missing and 11 partials ⚠️
cli/test/multiplier/txn.go	9.76%	37 Missing ⚠️
cli/test/action/collection_describe.go	40.48%	18 Missing and 7 partials ⚠️
cli/test/action/txn_action.go	0.00%	17 Missing ⚠️
cli/test/action/tx_create.go	0.00%	8 Missing ⚠️
cli/test/action/tx_commit.go	0.00%	6 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3550      +/-   ##
===========================================
+ Coverage    78.62%   78.64%   +0.02%     
===========================================
  Files          396      405       +9     
  Lines        36722    37019     +297     
===========================================
+ Hits         28872    29112     +240     
- Misses        6151     6177      +26     
- Partials      1699     1730      +31

Flag	Coverage Δ
all-tests	`78.64% <55.89%> (+0.02%)`	⬆️

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

Files with missing lines	Coverage Δ
cli/config.go	`80.00% <ø> (ø)`
cli/start.go	`45.83% <100.00%> (+18.62%)`	⬆️
cli/test/action/schema_add.go	`100.00% <100.00%> (ø)`
cli/test/action/start.go	`100.00% <100.00%> (ø)`
cli/test/integration/test.go	`100.00% <100.00%> (ø)`
cli/test/action/tx_commit.go	`0.00% <0.00%> (ø)`
cli/test/action/tx_create.go	`0.00% <0.00%> (ø)`
cli/test/action/txn_action.go	`0.00% <0.00%> (ø)`
cli/test/action/collection_describe.go	`40.48% <40.48%> (ø)`
cli/test/multiplier/txn.go	`9.76% <9.76%> (ø)`
... and 1 more

... and 21 files 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 acb43fa...16c02c4. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The default is 'none' as that is what the docs/config.md says it should be.

CrossLock only works when everything else is using it, and other test suites do not - this meant they were occassionally clashing with the CLI tests.

I have failed to find out why, but somehow, when the source-hub test job executes the cli tests it manages to change the default behaviour and start up defra with source-hub acp, which doesn't work as the keyring is disabled.

## Relevant issue(s) Resolves sourcenetwork#3551 ## Description Only give badger a path if not in-memory. Credit for fix belongs to Keenan not me. I broke this bringing corekv in. Keenan fixed this, I'm just doing the typing. This is tested in sourcenetwork#3550

## Relevant issue(s) Resolves sourcenetwork#3569 ## Description Adds the example CLI integration tests using new test framework. Largely as seen in the demo last week, but with a little more polish. I think it is good to commit these now and then we can expand them as/when we feel like, and use it as an example for other user-facing stuff (like http). I have not added the txn-commit multiplier to the CI, as the workflow test files are fairly coupled to executing all tests in the entire repository and I wanted to limit scope here. ## Tasks - [x] The testing repo dependency is currently a local, relative, path and needs to be pointed at https://github.com/sourcenetwork/testing when sourcenetwork/testo#1 gets merged. - [x] Link this to a proper issue if/when sourcenetwork/testo#1 is merged

AndrewSisley added the area/testing Related to any test or testing suite label Mar 19, 2025

AndrewSisley added this to the DefraDB v0.17 milestone Mar 19, 2025

AndrewSisley requested a review from a team March 19, 2025 16:27

AndrewSisley self-assigned this Mar 19, 2025

AndrewSisley mentioned this pull request Mar 19, 2025

feat: Initial version sourcenetwork/testo#1

Merged

1 task

AndrewSisley force-pushed the 3524-action-spike branch from ebe5d14 to d78645e Compare March 19, 2025 17:12

AndrewSisley mentioned this pull request Mar 19, 2025

fix: Only give badger a path if not in-memory #3552

Merged

AndrewSisley force-pushed the 3524-action-spike branch 2 times, most recently from 816afa2 to 6d510ab Compare March 24, 2025 21:21

islamaliev approved these changes Mar 24, 2025

View reviewed changes

AndrewSisley force-pushed the 3524-action-spike branch 4 times, most recently from 80d40ed to 61dc9d1 Compare March 25, 2025 16:35

AndrewSisley force-pushed the 3524-action-spike branch 12 times, most recently from 7b33d47 to a0581fb Compare March 26, 2025 16:52

AndrewSisley marked this pull request as ready for review March 26, 2025 17:46

AndrewSisley requested a review from islamaliev March 26, 2025 17:48

AndrewSisley requested a review from a team March 26, 2025 17:48

AndrewSisley added 9 commits March 28, 2025 15:30

Add start param for acp type

f14e856

The default is 'none' as that is what the docs/config.md says it should be.

Add example CLI integration tests

30b086f

PR FIXUP - Argment => Augment

e6b5f46

PR FIXUP - Simplify execute func

977e101

PR FIXUP - Document txn-commit multiplier

e4cc818

PR FIXUP - Add missing word

2c74972

PR FIXUP - Remove state.Cancels prop

c96a211

PR FIXUP - Read port from logs instead of crossLock

015c047

CrossLock only works when everything else is using it, and other test suites do not - this meant they were occassionally clashing with the CLI tests.

PR FIXUP - Expicitly set ACP type

77d0e9d

I have failed to find out why, but somehow, when the source-hub test job executes the cli tests it manages to change the default behaviour and start up defra with source-hub acp, which doesn't work as the keyring is disabled.

AndrewSisley force-pushed the 3524-action-spike branch 2 times, most recently from f1d6163 to bb215b0 Compare March 28, 2025 20:07

PR FIXUP - Update docs

16c02c4

AndrewSisley force-pushed the 3524-action-spike branch from bb215b0 to 16c02c4 Compare March 28, 2025 20:07

AndrewSisley merged commit 4dc4d01 into sourcenetwork:develop Mar 31, 2025
44 of 45 checks passed

AndrewSisley deleted the 3524-action-spike branch March 31, 2025 14:30

test: Add example CLI integration tests #3550

test: Add example CLI integration tests #3550

Uh oh!

Conversation

AndrewSisley commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Relevant issue(s)

Description

Tasks

Uh oh!

islamaliev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewSisley Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewSisley Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewSisley Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewSisley Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewSisley Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewSisley Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AndrewSisley commented Mar 19, 2025 •

edited

Loading

AndrewSisley Mar 25, 2025 •

edited

Loading

AndrewSisley Mar 25, 2025 •

edited

Loading

AndrewSisley Mar 25, 2025 •

edited

Loading

AndrewSisley Mar 25, 2025 •

edited

Loading

AndrewSisley Mar 25, 2025 •

edited

Loading

AndrewSisley Mar 25, 2025 •

edited

Loading

codecov bot commented Mar 25, 2025 •

edited

Loading