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

Add k6 new subcommand #3394

Merged
merged 15 commits into from
Nov 6, 2023
Merged

Add k6 new subcommand #3394

merged 15 commits into from
Nov 6, 2023

Conversation

andrewslotin
Copy link
Contributor

What?

This PR adds a new subcommand to k6 that creates a new test script file and populates it with a sample script similar to the one in Grafana Cloud k6 script editor.

Why?

This subcommand reduces the amount of boilerplate code required to author a new script. Sample code provides new k6 users with instructions on how to run this test in Grafana Cloud and/or enable Browser API by pointing them to appropriate places in documentation.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make ci-like-lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Closes #3294

@github-actions github-actions bot requested review from codebien and oleiade October 13, 2023 14:17
@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (0ff7078) 73.30% compared to head (f389ed3) 73.35%.

❗ Current head f389ed3 differs from pull request most recent head 28c3403. Consider uploading reports for the commit 28c3403 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3394      +/-   ##
==========================================
+ Coverage   73.30%   73.35%   +0.05%     
==========================================
  Files         260      261       +1     
  Lines       19687    19739      +52     
==========================================
+ Hits        14432    14480      +48     
  Misses       4365     4365              
- Partials      890      894       +4     
Flag Coverage Δ
ubuntu 73.28% <88.67%> (+0.04%) ⬆️
windows 73.19% <88.67%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
cmd/root.go 94.00% <100.00%> (ø)
cmd/new.go 88.46% <88.46%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrewslotin andrewslotin requested a review from dgzlopes October 13, 2023 15:42
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

Hey @andrewslotin,
thanks for addressing it 🙇

cmd/init.go Outdated Show resolved Hide resolved
cmd/init.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/init.go Outdated Show resolved Hide resolved
cmd/init.go Outdated Show resolved Hide resolved
cmd/init.go Outdated Show resolved Hide resolved
cmd/init.go Outdated Show resolved Hide resolved
cmd/init.go Outdated Show resolved Hide resolved
cmd/init.go Outdated Show resolved Hide resolved
cmd/init.go Outdated Show resolved Hide resolved
@codebien codebien added this to the v0.48.0 milestone Oct 16, 2023
@codebien codebien added the documentation-needed A PR which will need a separate PR for documentation label Oct 16, 2023
.gitignore Show resolved Hide resolved
@codebien
Copy link
Contributor

@mstoykov @olegbespalov Adding you so you are aware of it, as I know there are different opinions on this command.

oleiade
oleiade previously approved these changes Oct 18, 2023
cmd/init.go Outdated Show resolved Hide resolved
cmd/init.go Outdated Show resolved Hide resolved
Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

Looks good. I left a few minor comments and one suggestion 👍

cmd/init.go Outdated Show resolved Hide resolved
cmd/init.go Outdated Show resolved Hide resolved
cmd/init.go Outdated Show resolved Hide resolved
cmd/init.go Outdated Show resolved Hide resolved
codebien
codebien previously approved these changes Oct 18, 2023
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

I'm fine with most of the changes as I commented before I would prefer if we not include here the Cloud suggestion with the old option.

cmd/init.go Outdated Show resolved Hide resolved
olegbespalov
olegbespalov previously approved these changes Oct 18, 2023
@na--
Copy link
Member

na-- commented Nov 1, 2023

Hey, spotted this by chance when I was skimming notification emails, and I was immediately confused by the PR title, specifically about what is being initialized by k6 init 😅 Considering we have an "init context" in the test lifecycle, I thought it had something to do with that or even #785 😅

My 2c: having such a sub-command to is probably a good idea. Though I very rarely make use of such commands in other tools, I know other people find them useful. However, the k6 init sub-command might be somewhat confusing because we already have other things that we call "init". k6 create or k6 new or something like that seems less likely to be confusing.

@andrewslotin
Copy link
Contributor Author

I like the idea of k6 new! Indeed, while init seems to be widely adopted by other tools, it may convey false assumption that it can be used only once per project, while the idea is to have it a standard way to create a new script.

@olegbespalov, @codebien, what are your thoughts on this? I’d be up for making necessary renamings.

@codebien
Copy link
Contributor

codebien commented Nov 1, 2023

what are your thoughts on this?

I don't have a strong opinion here, k6 new has the benefit of being shorter so I am fine with it.

@andrewslotin andrewslotin dismissed stale reviews from olegbespalov and codebien via 5471985 November 3, 2023 11:24
@andrewslotin andrewslotin changed the title Add k6 init subcommand Add k6 new subcommand Nov 3, 2023
@andrewslotin andrewslotin requested a review from codebien November 3, 2023 11:25
@andrewslotin andrewslotin requested a review from oleiade November 3, 2023 11:25
@andrewslotin
Copy link
Contributor Author

andrewslotin commented Nov 3, 2023

Renamed k6 init to k6 new as per discussion above. @oleiade, @codebien, PTAL at the last commits. There are no other changes apart from renaming the file and replacing init with new where appropriate.

@andrewslotin
Copy link
Contributor Author

OK, there is one more edit: I've replaced the link to k6.io/docs with Grafana docs ones, since we're migrating our documentation there.

Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

I don't have a strong opinion about the init vs. new.

Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

I'm quite happy with the final solution. Thanks for the effort. 🙇 @andrewslotin

Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

🚀 🚀 🚀

@andrewslotin andrewslotin merged commit e0827b4 into master Nov 6, 2023
21 checks passed
@andrewslotin andrewslotin deleted the add_k6_init branch November 6, 2023 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-needed A PR which will need a separate PR for documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A subcommand to initialize a new test script file
6 participants