Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve local test setup #177

Merged
merged 8 commits into from
Oct 16, 2024
Merged

Improve local test setup #177

merged 8 commits into from
Oct 16, 2024

Conversation

samlaf
Copy link
Collaborator

@samlaf samlaf commented Oct 8, 2024

Fixes Issue

Fixes #175

at least fixes everything with e2e tests. holesky tests are still not runnable locally until we get that private key stored somewhere.

Changes proposed

Screenshots (Optional)

Note to reviewers

@samlaf samlaf requested review from bxue-l2, teddyknox and epociask and removed request for teddyknox October 8, 2024 17:39
Copy link
Collaborator

@epociask epociask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally lgtm - just a few minor q's and knits

e2e/setup.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
.vscode/settings.json Show resolved Hide resolved
e2e/main_test.go Outdated Show resolved Hide resolved
e2e/setup.go Outdated
Comment on lines 50 to 59
func init() {
err := startMinioContainer()
if err != nil {
panic(err)
}
err = startRedisContainer()
if err != nil {
panic(err)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

knit - using an init pattern like this could make our code super hard to import - at least this library

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree that this sucks... also it could at some point be needed to have new clean instances per test. wanted to talk with you about how to best refactor this.

I didn't see an obvious place where to start these containers. The separation between creating the config (one fct that sets all the config vars) and starting the endpoints makes it hard... because the actual endpoint of the containers are only known after they are started, so we'd need to start them in the "createConfig" function, which feels wrong... perhaps we need yet another function like startDependencyContainers() or something? And we can call it from the test main function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On another note though I'm not sure why anyone would want to import this package... do you have anything in mind or just saying in case?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just saying as an in-case - maybe one day we try importing this into node software directly for cross testing framework integration

Copy link
Collaborator Author

@samlaf samlaf Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with you, but want to move on to other things for now and worry about this when it actually becomes a problem (perhaps an obvious refactor will also surface once we have an actual need for this). Added a TODO comment: a9ae57b

e2e/setup.go Show resolved Hide resolved
Copy link
Collaborator

@epociask epociask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - just very knits

minioEndpoint = strings.TrimPrefix(endpoint, "http://")
return nil
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

knit - we document the above function but not this one

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e2e/setup.go Outdated
}

// startMinioContainer starts a MinIO container and returns the container instance and its endpoint
func startMinioContainer() error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

knit - startMinIOContainer

Copy link
Collaborator Author

@samlaf samlaf Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samlaf samlaf merged commit df3a6f8 into main Oct 16, 2024
7 checks passed
@samlaf samlaf deleted the improve-local-test-setup branch October 16, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve e2e tests
2 participants