Add integration tests #274

amishas157 · 2024-08-05T21:29:12Z

PR Checklist

PR Structure

This PR has reasonably narrow scope (if not, break it down into smaller PRs).
This PR avoids mixing refactoring changes with feature changes (split into two PRs
otherwise).
This PR's title starts with the jira ticket associated with the PR.

Thoroughness

This PR adds tests for the most critical parts of the new functionality or fixes.
I've updated the README with the added features, breaking changes, new instructions on how to use the repository. I updated the description of the fuction with the changes that were made.

Release planning

I've decided if this PR requires a new major/minor/patch version accordingly to
semver, and I've changed the name of the BRANCH to release/_ , feature/_ or patch/* .

What

This PR adds integration tests for all the export_* commands. The changes comprise of following:

Existing runCLITest in cmd/export_ledgers_test.go is used to run all test files in cmd.
update=true flag is supported to update golden files automatically.
More tests suites are added for export_ledger_entry_changes_test. This is to cover all kinds of ledger entry changes listed in https://github.com/stellar/stellar-etl?tab=readme-ov-file#unbounded-currently-unsupported
Updated all the outdated golden files.
A docker file is added to run tests. Current docker file does not have context of /cmd.
A docker-compose file is added to start a docker compose service with right gcp creds and run the tests.
Github workflow is added to run integration test, find code coverage and a threshold check for coverage
Updated github workflows to cancel any previous jobs if new commits are pushed
Updated Readme and Makefile for helper with running the tests.
A secret CREDS_TEST_HUBBLE is added to this repo which has access to bigquery and storage bucket. This is used by integration test to read ledger backend data

Why

To gain confidence in feature addition or bug fix, integration tests are required.

Known limitations

None

Test Results:

Running integration test takes ~3 minutes.
Our current test coverage is 57% and I have setup a threshold of 55%
Commit intentionally failed to verify failure 1d39d47

amishas157 · 2024-08-06T22:59:52Z

How I ran the tests:

docker-compose build
docker-compose run integration-tests go test -run ^TestExport ./cmd -timeout 30m -args -update=true

add secret as env variable in github workflow remove env from docker compose

mount file and pass env remove extra env temp cat temp empty printf rearrange validate json workaround cleanup verbose

This reverts commit 1d39d47.

This reverts commit b797bdf.

Fix coverage report coverage fix

amishas157 · 2024-08-08T22:41:55Z

cmd/export_ledgers_test.go

-			wantErr: fmt.Errorf("could not read ledgers: End sequence number is less than start (50 < 100)"),
-		},
-		{
-			name:    "start too large",
-			args:    []string{"export_ledgers", "-s", "4294967295", "-e", "4294967295"},
-			golden:  "",
-			wantErr: fmt.Errorf("could not read ledgers: Latest sequence number is less than start sequence number (%d < 4294967295)", latestLedger),
-		},
-		{
-			name:    "end too large",
-			args:    []string{"export_ledgers", "-e", "4294967295", "-l", "4294967295"},
-			golden:  "",
-			wantErr: fmt.Errorf("could not read ledgers: Latest sequence number is less than end sequence number (%d < 4294967295)", latestLedger),


These are flaky test. Therefore I removed them

amishas157 · 2024-08-08T22:42:38Z

cmd/export_ledgers_test.go

+		idxOfOutputArg := indexOf(test.args, "-o")
+		var testOutput []byte
+		var outLocation string
+		var stat os.FileInfo
+		if idxOfOutputArg > -1 {
+			outLocation = test.args[idxOfOutputArg+1]
+			_, err = os.Stat(outLocation)
+			if err != nil {
+				// Check if the error is due to the file not existing
+				if !os.IsNotExist(err) {
+					assert.NoError(t, err)
+				}
+			} else {
+				err = deleteLocalFiles(outLocation)
+				if err != nil {
+					log.Fatal(err)
+				}
+			}
+		}
+


This is to delete anything in the output directory before exporting any data.

sydneynotthecity

This looks great!

I have a couple questions for my own understanding before approving. I also glanced over the golden set files briefly, but i'm assuming they are valid since the integration tests succeed.

.github/workflows/integration-tests.yml

cmd/export_contract_events_test.go

cmd/export_ledger_entry_changes_test.go

sydneynotthecity · 2024-08-09T16:00:51Z

cmd/export_ledger_entry_changes_test.go

Do you think we'll run into any issues using different ledger ranges to generate these files?

That's a good point. There can be issues if code has bug for different kind of input. Since the ledger range can contain only certain variety of data. It may not cover for all test cases.

I had a thought to have custom fixtures along with ledger range test cases. But I think that's already part of our unit tests. so we should be good generally.

I think this is a good enough start point and if we run into edge cases, we can always either adjust the ledger ranges or add custom fixtures as you suggested

cmd/export_ledgers_test.go

cmd/export_ledger_entry_changes_test.go

cmd/export_ledgers_test.go

sydneynotthecity

LGTM

amishas157 added 9 commits August 5, 2024 16:08

Boiler plate

c391e61

Make the tests run

687ff9d

Truncate output file/dir before the export

8ea6e38

Make export transactions test run

6b7921f

Update the golden files

2b55e5b

prettier

b7c36b3

omit long running tests

b38fae5

Make all the tests work

9600e7b

prettier

3e2df01

amishas157 added 5 commits August 7, 2024 12:45

update

8a956c5

Make all the test run

4211b9e

Add workflow to run integration tests

9b95998

revert me

b2a3ae6

Fix docker compose in workflow

00056df

amishas157 force-pushed the integration-test branch from f2e6ca2 to 9dfec73 Compare August 8, 2024 14:49

use secret

f08b276

add secret as env variable in github workflow remove env from docker compose

amishas157 force-pushed the integration-test branch 8 times, most recently from 2eae7f7 to de8c133 Compare August 8, 2024 19:46

pass env directly in docker compose run

fcffd97

mount file and pass env remove extra env temp cat temp empty printf rearrange validate json workaround cleanup verbose

amishas157 force-pushed the integration-test branch from de8c133 to fcffd97 Compare August 8, 2024 19:53

amishas157 added 3 commits August 8, 2024 15:06

Fail the test intentionally

1d39d47

Revert "Fail the test intentionally"

31fb156

This reverts commit 1d39d47.

find test coverage

77c0027

amishas157 added 2 commits August 8, 2024 15:42

test overrides

b797bdf

Revert "test overrides"

eb0bfad

This reverts commit b797bdf.

amishas157 force-pushed the integration-test branch from 54c5ea6 to 0abc22c Compare August 8, 2024 21:55

add coverage report and readme

6b4d818

Fix coverage report coverage fix

amishas157 force-pushed the integration-test branch from 0abc22c to 6b4d818 Compare August 8, 2024 22:03

amishas157 added 4 commits August 8, 2024 17:08

lower our test coverage standard

9d1693c

Remove duplicate functions

dae96ac

Add make command for lint

dd3d1d4

Cancel the previous runs if new commit is pushed

d41df7a

amishas157 changed the title ~~Boiler plate~~ Add integration tests Aug 8, 2024

amishas157 marked this pull request as ready for review August 8, 2024 22:40

amishas157 requested a review from a team as a code owner August 8, 2024 22:40

amishas157 commented Aug 8, 2024

View reviewed changes

rename the job

6aac320

sydneynotthecity reviewed Aug 9, 2024

View reviewed changes

amishas157 commented Aug 9, 2024

View reviewed changes

cmd/export_ledger_entry_changes_test.go Outdated Show resolved Hide resolved

Update cmd/export_ledger_entry_changes_test.go

406f4dc

amishas157 commented Aug 9, 2024

View reviewed changes

cmd/export_ledgers_test.go Outdated Show resolved Hide resolved

amishas157 added 2 commits August 9, 2024 11:21

Update cmd/export_ledgers_test.go

b7442e1

Rename contract event golden file

b33d0d5

sydneynotthecity approved these changes Aug 9, 2024

View reviewed changes

amishas157 merged commit c7991a5 into master Aug 9, 2024
8 checks passed

sydneynotthecity mentioned this pull request Aug 15, 2024

investigate and fix cmd unit tests #131

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration tests #274

Add integration tests #274

amishas157 commented Aug 5, 2024 •

edited

Loading

amishas157 commented Aug 6, 2024

amishas157 Aug 8, 2024

amishas157 Aug 8, 2024

sydneynotthecity left a comment

sydneynotthecity Aug 9, 2024

amishas157 Aug 9, 2024

sydneynotthecity Aug 9, 2024

sydneynotthecity left a comment

Add integration tests #274

Add integration tests #274

Conversation

amishas157 commented Aug 5, 2024 • edited Loading

PR Structure

Thoroughness

Release planning

What

Why

Known limitations

Test Results:

amishas157 commented Aug 6, 2024

amishas157 Aug 8, 2024

Choose a reason for hiding this comment

amishas157 Aug 8, 2024

Choose a reason for hiding this comment

sydneynotthecity left a comment

Choose a reason for hiding this comment

sydneynotthecity Aug 9, 2024

Choose a reason for hiding this comment

amishas157 Aug 9, 2024

Choose a reason for hiding this comment

sydneynotthecity Aug 9, 2024

Choose a reason for hiding this comment

sydneynotthecity left a comment

Choose a reason for hiding this comment

amishas157 commented Aug 5, 2024 •

edited

Loading