-
-
Notifications
You must be signed in to change notification settings - Fork 511
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
feat(k6):Add remote test scripts #2350
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
In #2401 we added support for a Wdyt? |
@mdelapenya No problem, I can do that. |
Updated to use reader API |
} | ||
options = k6.WithTestScript(absPath) | ||
}else{ | ||
|
There was a problem hiding this comment.
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
func downloadFileFromDescription(d RemoteTestFileDescription) error{ | ||
|
||
client := http.Client{Timeout: time.Second*60} | ||
// Set up HTTPS request with basic authorization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment should go here: https://github.com/testcontainers/testcontainers-go/pull/2350/files#diff-19a4c472d6cc0154e74ab208f0a048d900eb0e6006b4365b853d56218d73cd5fR48, next to where it applies
modules/k6/k6.go
Outdated
return opt | ||
} | ||
|
||
func WithRemoteTestScript(d RemoteTestFileDescription) testcontainers.CustomizeRequestOption { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@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 { |
There was a problem hiding this comment.
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
type RemoteTestFileDescription struct { | |
type DownloadableFile struct { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
There was a problem hiding this 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
docs/modules/k6.md
Outdated
|
||
uri, _ := url.Parse(scriptUrl) | ||
desc := k6.DownloadableFile{Uri: *uri , DownloadDir: t.TempDir()} | ||
options = k6.WithRemoteTestScript(desc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options = k6.WithRemoteTestScript(desc) | |
options := k6.WithRemoteTestScript(desc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* 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) ...
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.