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

feat(k6):Add remote test scripts #2350

Merged
merged 10 commits into from
Apr 3, 2024

Conversation

bearrito
Copy link
Contributor

@bearrito bearrito commented Mar 13, 2024

What does this PR do?

Adds support for remote test scripts

Why is it important?

Addressing #2246

How to test this PR

Run the unit tests.

I'm dogfooding the existing scripts from within the git repo iteself. I think this is a good idea because we will know the existing scripts pass/fail, then if they change it will be automatically reflected in the remote tests.

I don't have a server setup with basic auth for me to test the auth portion.

@bearrito bearrito requested a review from a team as a code owner March 13, 2024 19:56
Copy link

netlify bot commented Mar 13, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit fead61a
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/660d70cb9ac1210008f7b2c3
😎 Deploy Preview https://deploy-preview-2350--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mdelapenya mdelapenya self-assigned this Mar 18, 2024
@mdelapenya
Copy link
Collaborator

In #2401 we added support for a io.Reader in the ContainerFile struct. I think we could leverage it in this module, changing the internals of the WithTestScript function to get the reader. We could have a WithTestScriptReader function that receives the reader, and that reader is passed to the request's files. Finally, I'd change the existing option to invoke the reader with the file reader.

Wdyt?

@bearrito
Copy link
Contributor Author

@mdelapenya No problem, I can do that.

@bearrito
Copy link
Contributor Author

Updated to use reader API

modules/k6/k6.go Outdated Show resolved Hide resolved
modules/k6/k6.go Outdated Show resolved Hide resolved
modules/k6/k6.go Outdated Show resolved Hide resolved
modules/k6/k6.go Outdated Show resolved Hide resolved
modules/k6/k6.go Show resolved Hide resolved
modules/k6/k6.go Show resolved Hide resolved
modules/k6/k6.go Outdated Show resolved Hide resolved
modules/k6/k6.go Outdated Show resolved Hide resolved
modules/k6/k6_test.go Outdated Show resolved Hide resolved
}
options = k6.WithTestScript(absPath)
}else{

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

modules/k6/k6_test.go Outdated Show resolved Hide resolved
modules/k6/k6.go Outdated Show resolved Hide resolved
modules/k6/k6.go Outdated
func downloadFileFromDescription(d RemoteTestFileDescription) error{

client := http.Client{Timeout: time.Second*60}
// Set up HTTPS request with basic authorization.
Copy link
Collaborator

Choose a reason for hiding this comment

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

modules/k6/k6.go Outdated Show resolved Hide resolved
modules/k6/k6.go Outdated Show resolved Hide resolved
modules/k6/k6.go Outdated
return opt
}

func WithRemoteTestScript(d RemoteTestFileDescription) testcontainers.CustomizeRequestOption {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add function comments here too. I'd think about if we need to validate the URL here 🤔 Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll use net/URL. I'll assume if the URL parses it's valid for the user's use case. I guess we could also verify on the extension? But I'm not sure what types are allowed for K6

modules/k6/k6.go Outdated Show resolved Hide resolved
@bearrito
Copy link
Contributor Author

@mdelapenya I think I addressed all your comments.

modules/k6/k6.go Outdated
@@ -17,17 +22,73 @@ type K6Container struct {
testcontainers.Container
}

type RemoteTestFileDescription struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wdyt if we make this name more obvious? I'd then rename https://github.com/testcontainers/testcontainers-go/pull/2350/files#diff-19a4c472d6cc0154e74ab208f0a048d900eb0e6006b4365b853d56218d73cd5fR38 to just func downloadFile

Suggested change
type RemoteTestFileDescription struct {
type DownloadableFile struct {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// WithTestScriptReader copies files into the Container using the Reader API
// The script base name is not a path, neither absolute or relative and should
// be just the file name of the script
func WithTestScriptReader(reader io.Reader, scriptBaseName string) testcontainers.CustomizeRequestOption {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We must include these options in the docs (root-dir/docs/modules/k6.md). Let's include that it will only accept Javascript files and the configuration of the http client (timeout=60s, context type, etc) https://github.com/testcontainers/testcontainers-go/pull/2350/files#diff-19a4c472d6cc0154e74ab208f0a048d900eb0e6006b4365b853d56218d73cd5fR40-R50

The users of the module would like to know this option exists and how to use it, from our docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

mdelapenya
mdelapenya previously approved these changes Apr 3, 2024
Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

There is only one typo in the code snippet for the docs. Once fixed, I'll merge it


uri, _ := url.Parse(scriptUrl)
desc := k6.DownloadableFile{Uri: *uri , DownloadDir: t.TempDir()}
options = k6.WithRemoteTestScript(desc)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
options = k6.WithRemoteTestScript(desc)
options := k6.WithRemoteTestScript(desc)

@mdelapenya mdelapenya added the feature New functionality or new behaviors on the existing one label Apr 3, 2024
@mdelapenya mdelapenya changed the title feat(remotek6):Add remote test scripts feat(k6):Add remote test scripts Apr 3, 2024
Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM!

@mdelapenya mdelapenya merged commit 55ca42a into testcontainers:main Apr 3, 2024
101 of 102 checks passed
mdelapenya added a commit to coffeegoddd/testcontainers-go that referenced this pull request Apr 12, 2024
* main: (115 commits)
  chore: create TLS certs in a consistent manner (testcontainers#2478)
  chore(deps): bump idna from 3.6 to 3.7 (testcontainers#2480)
  Elasticsearch disable CA retrieval when ssl is disabled (testcontainers#2475)
  fix: handle dockerignore exclusions properly (testcontainers#2476)
  chore: prepare for next minor development cycle (0.31.0)
  chore: use new version (v0.30.0) in modules and examples
  Fix url creation to handle query params when using HTTP wait strategy (testcontainers#2466)
  fix: data race on container run (testcontainers#2345)
  fix: logging deadlock (testcontainers#2346)
  feat(k6):Add remote test scripts (testcontainers#2350)
  feat: optimizes file copies to and from containers (testcontainers#2450)
  fix(exec): updates the `Multiplexed` opt to combine stdout and stderr (testcontainers#2452)
  Upgrade neo4j module to use features from v0.29.1 of testcontainers-go (testcontainers#2463)
  bug:Fix AMQPS url (testcontainers#2462)
  chore: more compose updates in comments
  chore: use "docker compose" (v2) instead of "docker-compose" (v1) (testcontainers#2464)
  chore(deps): bump github/codeql-action from 2.22.12 to 3.24.9 (testcontainers#2459)
  refactor: Add Weaviate modules tests (testcontainers#2447)
  feat(exitcode): Add exit code sugar method (testcontainers#2342)
  feat: add module to support InfluxDB v1.x (testcontainers#1703)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or new behaviors on the existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants