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

K6 module #1721

Merged
merged 18 commits into from Oct 9, 2023
Merged

K6 module #1721

merged 18 commits into from Oct 9, 2023

Conversation

pablochacin
Copy link
Contributor

@pablochacin pablochacin commented Oct 3, 2023

What does this PR do?

Implements a module for running k6.

Why is it important?

K6 is a well-known tool for testing. This module allows the reuse of existing k6 test scripts in tests using TestContainers. This is particularly interesting when implementing e2e tests.

Note to reviewers

There are a few things I'm not totally satisfied with in its present form, and I would like to address in future iterations:

  • The test script passed to k6 is presently dfined as an option (WithTestScript), but it reality a "mandatory option" and if not specified, the test execution will fail. I considered adding it as an argument to the RunContainer function, but this seems not to be how other modules work.
RunContainer(ctx context.Context, string script, options....)
  • Specifying a build cache also requires running the container with the AsUser option to give it access to the cache directory. This is required because the image runs with its own user and mounting a local directory would fail with an access error. Maybe there's an alternative way to do this. For example, if instead of using a local directory, the cache is passed as a volume, this wouldn't be a problem. But requiring the user to create a docker volume before the test seems a little overkill.

Edit: I found a way to mount a regular directory in the image without requiring specifying an user.

Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
@netlify
Copy link

netlify bot commented Oct 3, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 4413ca4
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/6523f5f157c12a00082c5320
😎 Deploy Preview https://deploy-preview-1721--testcontainers-go.netlify.app/modules/k6
📱 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.

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.

Hey @pablochacin great job with the module! Thank you so much!!

I added a few comments regarding some leftovers from previous runs, as @mmorel-35 pointed out (thank you), so let's remove the examples/k6 files and references.

I have one question regarding the test execution, probably caused by my total inexperience in the k6 project, so sorry about it if it's a dumb question: I can imagine the JS script will try to connect to an application to perform some load, right? Should the k6 module provide a way to connect to that app, or it's not needed? ATM I cannot picture how the script interacts with the app under test.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.vscode/.testcontainers-go.code-workspace Outdated Show resolved Hide resolved
docs/examples/k6.md Outdated Show resolved Hide resolved
examples/k6/Makefile Outdated Show resolved Hide resolved
modules/k6/examples_test.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_test.go Outdated Show resolved Hide resolved
sonar-project.properties Outdated Show resolved Hide resolved
@pablochacin
Copy link
Contributor Author

Hi @mdelapenya @mmorel-35 I'm adding a module and the example for that module.
Should I submit them separately? For me, it makes more sense to add both, as the example is part of the documentation.

@pablochacin
Copy link
Contributor Author

I have one question regarding the test execution, probably caused by my total inexperience in the k6 project, so sorry about it if it's a dumb question: I can imagine the JS script will try to connect to an application to perform some load, right? Should the k6 module provide a way to connect to that app, or it's not needed? ATM I cannot picture how the script interacts with the app under test.

@mdelapenya yes, that is the case. In general, the scripts use an environment variable to get the address of the target application. I was planning to add an example for this.

@mdelapenya
Copy link
Collaborator

Hi @mdelapenya @mmorel-35 I'm adding a module and the example for that module. Should I submit them separately? For me, it makes more sense to add both, as the example is part of the documentation.

Oh I see now. Then the docs are not clear enough. Our bad.

Please add an examples_test.go file (IIRC the generator should create it) in the k6 module dir, including the ExampleRunContainer function.

TBH I'm thinking about removing everything under examples, as they are just plain code snippets to copy paste, probably causing more confusion than benefits. Since we added testable examples support I think they are not needed anymore.

@pablochacin
Copy link
Contributor Author

Oh I see now. Then the docs are not clear enough. Our bad.

This PR is still a draft. The documentation and examples are still a work in progress.

TBH I'm thinking about removing everything under examples,

Then it makes sense to remove the examples. I will put all of them under the module's examples_test.go.

Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
@pablochacin
Copy link
Contributor Author

@mdelapenya I don't really understand this linter message. Could you please help me point out the issue? 🙏

Running [/home/runner/golangci-lint-1.54.1-linux-amd64/golangci-lint run --out-format=github-actions --path-prefix=modules/k6 --verbose] in [/home/runner/work/testcontainers-go/testcontainers-go/modules/k6] ...
Error: File is not gci-ed with --skip-generated -s standard -s default -s prefix(github.com/testcontainers) (gci)
Error: File is not gci-ed with --skip-generated -s standard -s default -s prefix(github.com/testcontainers) (gci)
Error: File is not gci-ed with --skip-generated -s standard -s default -s prefix(github.com/testcontainers) (gci)

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
@pablochacin
Copy link
Contributor Author

pablochacin commented Oct 4, 2023

It is weird that running the linter locally is not catching any of these errors. I'm not sure what I'm doing wrong.

I'm running it from my module's root directory (modules/k6) using the following command that is generating the output shown below

make lint 
golangci-lint run --out-format=github-actions --path-prefix=. --verbose -c /home/pablo/go/src/github.com/testcontainers/testcontainers-go/.golangci.yml --fix
INFO [config_reader] Used config file .golangci.yml 
INFO [lintersdb] Active 12 linters: [errcheck errorlint gci gocritic gofumpt gosimple govet ineffassign misspell staticcheck typecheck unused] 
INFO [loader] Go packages loading at mode 575 (files|name|compiled_files|deps|types_sizes|exports_file|imports) took 339.074702ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 25.921286ms 
INFO [linters_context/goanalysis] analyzers took 0s with no stages 
INFO [runner] fixer took 0s with no stages        
INFO [runner] Issues before processing: 97, after processing: 0 
INFO [runner] Processors filtering stat (out/in): skip_files: 97/97, autogenerated_exclude: 97/97, identifier_marker: 97/97, cgo: 97/97, filename_unadjuster: 97/97, skip_dirs: 97/97, path_prettifier: 97/97, exclude: 97/97, exclude-rules: 4/97, nolint: 0/4 
INFO [runner] processing took 8.156372ms with stages: nolint: 4.288631ms, identifier_marker: 2.089663ms, exclude-rules: 601.775µs, path_prettifier: 569.636µs, autogenerated_exclude: 463.261µs, skip_dirs: 72.157µs, fixer: 40.755µs, cgo: 15.52µs, filename_unadjuster: 7.481µs, max_same_issues: 1.853µs, source_code: 740ns, skip_files: 705ns, diff: 605ns, exclude: 600ns, severity-rules: 545ns, uniq_by_line: 505ns, path_shortener: 497ns, max_from_linter: 442ns, sort_results: 423ns, max_per_file_from_linter: 292ns, path_prefixer: 286ns 
INFO [runner] linters took 585.581845ms with stages: goanalysis_metalinter: 577.308068ms 
INFO File cache stats: 0 entries of total size 0B 
INFO Memory: 11 samples, avg is 56.1MB, max is 88.5MB 
INFO Execution took 961.363062ms  

Something that I don't understand is this message because I don't see any change in any file after running the command.

INFO [runner] Issues before processing: 97, after processing: 0

@mmorel-35
Copy link
Contributor

@pablochacin ,

can you remove --path-prefix=. and see how it goes ?

@pablochacin
Copy link
Contributor Author

pablochacin commented Oct 4, 2023

can you remove --path-prefix=. and see how it goes ?

Same 😞 I mean, no changes.

@mmorel-35
Copy link
Contributor

When I try it on codespaces from artemis module's where I have created a deliberated issue

/workspaces/testcontainers-go/modules/artemis $ make lint
golangci-lint run --out-format=github-actions --path-prefix=. --verbose -c /workspaces/testcontainers-go/.golangci.yml --fix
INFO [config_reader] Used config file ../../.golangci.yml
INFO [lintersdb] Active 11 linters: [errcheck errorlint gci gocritic gofumpt gosimple govet ineffassign misspell staticcheck unused] 
INFO [loader] Go packages loading at mode 575 (types_sizes|files|imports|name|compiled_files|deps|exports_file) took 245.382898ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 511.104µs 
INFO [linters_context/goanalysis] analyzers took 0s with no stages 
INFO [runner] Fix issue &result.Issue{FromLinter:"gci", Text:"File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/testcontainers)", Severity:"", SourceLines:[]string{"\t\"github.com/docker/go-connections/nat\""}, Replacement:(*result.Replacement)(0xc00114a6f0), Pkg:(*packages.Package)(0xc000f9f500), LineRange:(*result.Range)(nil), Pos:token.Position{Filename:"artemis.go", Offset:0, Line:7, Column:0}, HunkPos:0, ExpectNoLint:false, ExpectedNoLintLinter:""} with range {7 7} 
INFO [runner] fixer took 1.232051ms with stages: all: 1.232051ms 
INFO [runner] Issues before processing: 3, after processing: 0 
INFO [runner] Processors filtering stat (out/in): source_code: 1/1, path_shortener: 1/1, filename_unadjuster: 3/3, autogenerated_exclude: 3/3, exclude: 3/3, nolint: 1/1, uniq_by_line: 1/1, diff: 1/1, identifier_marker: 3/3, max_same_issues: 1/1, severity-rules: 1/1, cgo: 3/3, path_prettifier: 3/3, skip_files: 3/3, skip_dirs: 3/3, max_from_linter: 1/1, fixer: 0/1, exclude-rules: 1/3, max_per_file_from_linter: 1/1 
INFO [runner] processing took 1.745278ms with stages: fixer: 1.258429ms, nolint: 198.761µs, identifier_marker: 92.634µs, exclude-rules: 66.093µs, path_prettifier: 52.589µs, autogenerated_exclude: 43.16µs, source_code: 15.779µs, skip_dirs: 11.432µs, path_shortener: 1.483µs, cgo: 1.172µs, max_same_issues: 772ns, filename_unadjuster: 561ns, uniq_by_line: 391ns, exclude: 371ns, sort_results: 301ns, max_from_linter: 280ns, severity-rules: 280ns, skip_files: 260ns, max_per_file_from_linter: 230ns, diff: 170ns, path_prefixer: 130ns 
INFO [runner] linters took 457.480433ms with stages: goanalysis_metalinter: 455.644814ms 
INFO File cache stats: 1 entries of total size 3.0KiB 
INFO Memory: 9 samples, avg is 52.1MB, max is 84.7MB 
INFO Execution took 707.979266ms

See the relative path to the "Used config file ../../.golangci.yml" is different from your output. Any reason ?

@mdelapenya
Copy link
Collaborator

When I fetched the PR before 420041c, I saw errors in the command output with the same lines that GH added as comments in the PR.

Basically saving the k6.go file on my VSCode fixed it, as it ran the automatic format for Go.

Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
docs/modules/k6.md Outdated Show resolved Hide resolved
@pablochacin pablochacin requested a review from a team as a code owner October 6, 2023 08:33
@pablochacin
Copy link
Contributor Author

pablochacin commented Oct 6, 2023

Hi I'm submitting this first version of the k6 module. Thanks, @mdelapenya and @mmorel-35 for your reviews of the draft. I marked all your suggested changes as resolved to facilitate processing any new comments.

@pablochacin
Copy link
Contributor Author

pablochacin commented Oct 6, 2023

Update: Just after submitting the PR, I realized there is a way to specify the cache dir without requiring the user. I would prefer to make the changes in this PR because it really improves the user experience. What do you think?

Update: My comment above was premature. Further testing has shown the result is not as expected. So please ignore this. Sorry for the noise.

@pablochacin
Copy link
Contributor Author

Hi. What is the preferred way to update a PR when it is out-of-date with the base branch? merge commit or rebase?

Screenshot at 2023-10-09 09-37-34

@mmorel-35
Copy link
Contributor

I would recommend with rebase when it's possible.

@mdelapenya mdelapenya self-assigned this Oct 9, 2023
@mdelapenya mdelapenya added feature New functionality or new behaviors on the existing one hacktoberfest Pull Requests accepted for Hacktoberfest. labels Oct 9, 2023
@mdelapenya
Copy link
Collaborator

mdelapenya commented Oct 9, 2023

I would recommend with rebase when it's possible.

I usually use the merge commit strategy, as I see it more convenient regarding keeping the git history and commit IDs. I understand that rebasing is "cleaner" in terms of git history, but at the same time, moves the pointer of when the first commit was created, probably removing context about that time: e.g. at the time the PR was created, a dependency was not updated yet, therefore the new module could not include it.

Said that, I'd be very happy in a change to rebase strategy if there are advantages that I'm not aware of, so please help me with that if you consider rebasing is better.

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.

I think we are super close to be ready. Only concerned about the usage of Mounts Vs Files (see comments), which we should explain in our docs in a better way.

Other than that, once addressed, we are ready to go

Thank you for your time here!

modules/k6/examples_test.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/examples_test.go Show resolved Hide resolved
modules/k6/k6.go Outdated Show resolved Hide resolved
modules/k6/examples_test.go Show resolved Hide resolved
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
modules/k6/k6.go Outdated Show resolved Hide resolved
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
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.

Thanks for this loong but properly discussed PR.

LGTM

@mdelapenya mdelapenya merged commit d2fe780 into testcontainers:main Oct 9, 2023
110 checks passed
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Oct 13, 2023
* main:
  docs: Fix typo and mention the relevant function name in doc (testcontainers#1745)
  DOCKER_HOST var typo (testcontainers#1743)
  feat: Add Cassandra module (testcontainers#1726)
  K6 module (testcontainers#1721)
  Rancher Desktop instructions (testcontainers#1724)
  chore(deps): bump github.com/shirou/gopsutil/v3 from 3.23.8 to 3.23.9 (testcontainers#1720)
  chore(deps): bump urllib3 from 2.0.5 to 2.0.6 (testcontainers#1718)
  chore(deps): bump github.com/twmb/franz-go/pkg/kadm in /modules/redpanda (testcontainers#1714)
  chore(deps): bump github.com/couchbase/gocb/v2 in /modules/couchbase (testcontainers#1704)
  chore(deps): bump github.com/neo4j/neo4j-go-driver/v5 in /modules/neo4j (testcontainers#1713)
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Oct 16, 2023
* main: (137 commits)
  Fix wrong module names (testcontainers#1776)
  docs: add default options to k6 module (testcontainers#1744)
  fix race condition in Test_StartStop (testcontainers#1700)
  chore(deps): bump github.com/aws/aws-sdk-go-v2/{service/s3,credentials,config} in /modules/localstack (testcontainers#1773)
  chore(deps): bump cloud.google.com/go/{datastore,bigtable,spanner} in /modules/gcloud (testcontainers#1774)
  chore(deps): bump golang.org/x/net from 0.15.0 to 0.17.0 (testcontainers#1772)
  docs: Fix typo and mention the relevant function name in doc (testcontainers#1745)
  DOCKER_HOST var typo (testcontainers#1743)
  feat: Add Cassandra module (testcontainers#1726)
  K6 module (testcontainers#1721)
  Rancher Desktop instructions (testcontainers#1724)
  chore(deps): bump github.com/shirou/gopsutil/v3 from 3.23.8 to 3.23.9 (testcontainers#1720)
  chore(deps): bump urllib3 from 2.0.5 to 2.0.6 (testcontainers#1718)
  chore(deps): bump github.com/twmb/franz-go/pkg/kadm in /modules/redpanda (testcontainers#1714)
  chore(deps): bump github.com/couchbase/gocb/v2 in /modules/couchbase (testcontainers#1704)
  chore(deps): bump github.com/neo4j/neo4j-go-driver/v5 in /modules/neo4j (testcontainers#1713)
  chore(deps): bump github.com/nats-io/nats.go in /modules/nats (testcontainers#1705)
  chore(deps): bump cloud.google.com/go/firestore from 1.12.0 to 1.13.0, google.golang.org/api from 0.142.0 to 0.143.0 and cloud.google.com/ge, google.golang.org/api from 0.142.0 to 0.143.0 and cloud.google.com/go/bigquery from 1.53.0 to 1.55 in /modules/gcloud (testcontainers#1716)
  chore(deps): bump github.com/aws/aws-sdk-go-v2/service/s3 from 1.39.0 to 1.40.0 and github.com/aws/aws-sdk-go from 1.45.15 to 1.45.19 in /modules/localstack (testcontainers#1717)
  chore: prepare for next minor development cycle (0.26.0)
  ...
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Oct 16, 2023
* main: (137 commits)
  Fix wrong module names (testcontainers#1776)
  docs: add default options to k6 module (testcontainers#1744)
  fix race condition in Test_StartStop (testcontainers#1700)
  chore(deps): bump github.com/aws/aws-sdk-go-v2/{service/s3,credentials,config} in /modules/localstack (testcontainers#1773)
  chore(deps): bump cloud.google.com/go/{datastore,bigtable,spanner} in /modules/gcloud (testcontainers#1774)
  chore(deps): bump golang.org/x/net from 0.15.0 to 0.17.0 (testcontainers#1772)
  docs: Fix typo and mention the relevant function name in doc (testcontainers#1745)
  DOCKER_HOST var typo (testcontainers#1743)
  feat: Add Cassandra module (testcontainers#1726)
  K6 module (testcontainers#1721)
  Rancher Desktop instructions (testcontainers#1724)
  chore(deps): bump github.com/shirou/gopsutil/v3 from 3.23.8 to 3.23.9 (testcontainers#1720)
  chore(deps): bump urllib3 from 2.0.5 to 2.0.6 (testcontainers#1718)
  chore(deps): bump github.com/twmb/franz-go/pkg/kadm in /modules/redpanda (testcontainers#1714)
  chore(deps): bump github.com/couchbase/gocb/v2 in /modules/couchbase (testcontainers#1704)
  chore(deps): bump github.com/neo4j/neo4j-go-driver/v5 in /modules/neo4j (testcontainers#1713)
  chore(deps): bump github.com/nats-io/nats.go in /modules/nats (testcontainers#1705)
  chore(deps): bump cloud.google.com/go/firestore from 1.12.0 to 1.13.0, google.golang.org/api from 0.142.0 to 0.143.0 and cloud.google.com/ge, google.golang.org/api from 0.142.0 to 0.143.0 and cloud.google.com/go/bigquery from 1.53.0 to 1.55 in /modules/gcloud (testcontainers#1716)
  chore(deps): bump github.com/aws/aws-sdk-go-v2/service/s3 from 1.39.0 to 1.40.0 and github.com/aws/aws-sdk-go from 1.45.15 to 1.45.19 in /modules/localstack (testcontainers#1717)
  chore: prepare for next minor development cycle (0.26.0)
  ...
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 hacktoberfest Pull Requests accepted for Hacktoberfest.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants