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

Refactor new command #4153

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 34 additions & 105 deletions cmd/new.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,99 +2,33 @@

import (
"fmt"
"path"
"text/template"

"github.com/fatih/color"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
templates "go.k6.io/k6/cmd/newtemplates"
"go.k6.io/k6/cmd/state"
"go.k6.io/k6/lib/fsext"
)

const defaultNewScriptName = "script.js"

//nolint:gochecknoglobals
var defaultNewScriptTemplate = template.Must(template.New("new").Parse(`import http from 'k6/http';
import { sleep } from 'k6';

export const options = {
// A number specifying the number of VUs to run concurrently.
vus: 10,
// A string specifying the total duration of the test run.
duration: '30s',

// The following section contains configuration options for execution of this
// test script in Grafana Cloud.
//
// See https://grafana.com/docs/grafana-cloud/k6/get-started/run-cloud-tests-from-the-cli/
// to learn about authoring and running k6 test scripts in Grafana k6 Cloud.
//
// cloud: {
// // The ID of the project to which the test is assigned in the k6 Cloud UI.
// // By default tests are executed in default project.
// projectID: "",
// // The name of the test in the k6 Cloud UI.
// // Test runs with the same name will be grouped.
// name: "{{ .ScriptName }}"
// },

// Uncomment this section to enable the use of Browser API in your tests.
//
// See https://grafana.com/docs/k6/latest/using-k6-browser/running-browser-tests/ to learn more
// about using Browser API in your test scripts.
//
// scenarios: {
// // The scenario name appears in the result summary, tags, and so on.
// // You can give the scenario any name, as long as each name in the script is unique.
// ui: {
// // Executor is a mandatory parameter for browser-based tests.
// // Shared iterations in this case tells k6 to reuse VUs to execute iterations.
// //
// // See https://grafana.com/docs/k6/latest/using-k6/scenarios/executors/ for other executor types.
// executor: 'shared-iterations',
// options: {
// browser: {
// // This is a mandatory parameter that instructs k6 to launch and
// // connect to a chromium-based browser, and use it to run UI-based
// // tests.
// type: 'chromium',
// },
// },
// },
// }
};

// The function that defines VU logic.
//
// See https://grafana.com/docs/k6/latest/examples/get-started-with-k6/ to learn more
// about authoring k6 scripts.
//
export default function() {
http.get('https://test.k6.io');
sleep(1);
}
`))

type initScriptTemplateArgs struct {
ScriptName string
}

// newScriptCmd represents the `k6 new` command
type newScriptCmd struct {
gs *state.GlobalState
overwriteFiles bool
templateType string
projectID string
}

func (c *newScriptCmd) flagSet() *pflag.FlagSet {
flags := pflag.NewFlagSet("", pflag.ContinueOnError)
flags.SortFlags = false
flags.BoolVarP(&c.overwriteFiles, "force", "f", false, "Overwrite existing files")

flags.BoolVarP(&c.overwriteFiles, "force", "f", false, "overwrite existing files")
flags.StringVar(&c.templateType, "template", "minimal", "template type (choices: minimal, protocol, browser)")
flags.StringVar(&c.projectID, "project-id", "", "specify the Grafana Cloud project ID for the test")
return flags
}

func (c *newScriptCmd) run(cmd *cobra.Command, args []string) error { //nolint:revive
func (c *newScriptCmd) run(cmd *cobra.Command, args []string) error {

Check warning on line 31 in cmd/new.go

View workflow job for this annotation

GitHub Actions / lint

unused-parameter: parameter 'cmd' seems to be unused, consider removing or renaming it as _ (revive)
target := defaultNewScriptName
if len(args) > 0 {
target = args[0]
Expand All @@ -104,64 +38,59 @@
if err != nil {
return err
}

if fileExists && !c.overwriteFiles {
return fmt.Errorf("%s already exists, please use the `--force` flag if you want overwrite it", target)
return fmt.Errorf("%s already exists. Use the `--force` flag to overwrite", target)
dgzlopes marked this conversation as resolved.
Show resolved Hide resolved
}

fd, err := c.gs.FS.Create(target)
if err != nil {
return err
}
defer func() {
_ = fd.Close() // we may think to check the error and log
}()
defer fd.Close()

Check failure on line 49 in cmd/new.go

View workflow job for this annotation

GitHub Actions / lint

Error return value of `fd.Close` is not checked (errcheck)

if err := defaultNewScriptTemplate.Execute(fd, initScriptTemplateArgs{
ScriptName: path.Base(target),
}); err != nil {
tmpl, err := templates.GetTemplate(c.templateType)
if err != nil {
return err
}

valueColor := getColor(c.gs.Flags.NoColor || !c.gs.Stdout.IsTTY, color.Bold)
printToStdout(c.gs, fmt.Sprintf(
"Initialized a new k6 test script in %s. You can now execute it by running `%s run %s`.\n",
valueColor.Sprint(target),
c.gs.BinaryName,
target,
))
argsStruct := templates.TemplateArgs{
ScriptName: target,
EnableCloud: c.projectID != "",
ProjectID: c.projectID,
}

if err := templates.ExecuteTemplate(fd, tmpl, argsStruct); err != nil {
return err
}

fmt.Fprintf(c.gs.Stdout, "New script created: %s (%s template).\n", target, c.templateType)

Check failure on line 66 in cmd/new.go

View workflow job for this annotation

GitHub Actions / lint

Error return value of `fmt.Fprintf` is not checked (errcheck)
return nil
}

func getCmdNewScript(gs *state.GlobalState) *cobra.Command {
c := &newScriptCmd{gs: gs}

exampleText := getExampleText(gs, `
# Create a minimal k6 script in the current directory. By default, k6 creates script.js.
{{.}} new

# Create a minimal k6 script in the current directory and store it in test.js
{{.}} new test.js

# Overwrite existing test.js with a minimal k6 script
{{.}} new -f test.js`[1:])
exampleText := getExampleText(c.gs, `
# Create a minimal k6 script
dgzlopes marked this conversation as resolved.
Show resolved Hide resolved
$ {{.}} new --template minimal
# Overwrite an existing file with a protocol-based script
$ {{.}} new -f --template protocol test.js
# Create a cloud-ready script with a specific project ID
$ {{.}} new --project-id 12315`[1:])

initCmd := &cobra.Command{
Use: "new",
Use: "new [file]",
Short: "Create and initialize a new k6 script",
Long: `Create and initialize a new k6 script.

This command will create a minimal k6 script in the current directory and
store it in the file specified by the first argument. If no argument is
provided, the script will be stored in script.js.
Long: `Create and initialize a new k6 script using one of the predefined templates.

This command will not overwrite existing files.`,
By default, the script will be named script.js unless a different name is specified.`,
Example: exampleText,
Args: cobra.MaximumNArgs(1),
RunE: c.run,
}
initCmd.Flags().AddFlagSet(c.flagSet())

initCmd.Flags().AddFlagSet(c.flagSet())
return initCmd
}
27 changes: 25 additions & 2 deletions cmd/new_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package cmd

import (
"fmt"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -56,7 +55,6 @@

jsData := string(data)
assert.Contains(t, jsData, "export const options = {")
assert.Contains(t, jsData, fmt.Sprintf(`name: "%s"`, testCase.expectedCloudName))
assert.Contains(t, jsData, "export default function() {")
})
}
Expand Down Expand Up @@ -95,3 +93,28 @@
assert.Contains(t, string(data), "export const options = {")
assert.Contains(t, string(data), "export default function() {")
}

func TestNewScriptCmd_InvalidTemplateType(t *testing.T) {
t.Parallel()

ts := tests.NewGlobalTestState(t)
ts.CmdArgs = []string{"k6", "new", "--template", "invalid-template"}

ts.ExpectedExitCode = -1

newRootCommand(ts.GlobalState).execute()
assert.Contains(t, ts.Stderr.String(), "invalid template type")
}

Check failure on line 107 in cmd/new_test.go

View workflow job for this annotation

GitHub Actions / lint

File is not `gofumpt`-ed (gofumpt)
func TestNewScriptCmd_ProjectID(t *testing.T) {
t.Parallel()

ts := tests.NewGlobalTestState(t)
ts.CmdArgs = []string{"k6", "new", "--project-id", "1422"}

newRootCommand(ts.GlobalState).execute()

data, err := fsext.ReadFile(ts.FS, defaultNewScriptName)
require.NoError(t, err)

assert.Contains(t, string(data), "projectID: 1422")
}
54 changes: 54 additions & 0 deletions cmd/newtemplates/browser.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { browser } from "k6/browser";
import http from "k6/http";
import { sleep, check } from 'k6';

const BASE_URL = __ENV.BASE_URL || "https://quickpizza.grafana.com";

export const options = {
scenarios: {
ui: {
executor: "shared-iterations",
Copy link
Contributor

Choose a reason for hiding this comment

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

I see in other examples we set duration/vus, should we set some "scope" here as well?
Otherwise, does it bring any value to define the executor?

Copy link
Member

Choose a reason for hiding this comment

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

👍🏻 Furthermore, my understanding is that in general users tend to run browser tests with 1 VU/1 iteration, at least when they get started. I know it's the default anyway, but as we're all addressing beginners with this command, it could be helpful to remove as much implicit as possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense! I have added VUs/Iterations.

Otherwise, does it bring any value to define the executor?

It is needed to be able to run a browser test. You must use scenarios, and scenarios must have an executor.

Btw... we do this same thing in lots of other places; maybe it is something we should change? cc @ankur22
e.g. https://grafana.com/docs/k6/latest/using-k6-browser/#a-simple-browser-test

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean adding the VU/iteration to all the other examples too? The situation with scenarios and browser tests isn't great tbh, and being forced to add an executor. I think we wanted to avoid making a very large options block, and the minimal is:

export const options = {
  scenarios: {
    ui: {
      executor: 'shared-iterations',
      options: {
        browser: {
          type: 'chromium',
        },
      },
    },
  },
};

Adding iteration and vus adds a couple more lines to it.

If the general consensus is that it makes is clearer for new users then it's something to consider. CC @inancgumus

Copy link
Member

Choose a reason for hiding this comment

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

I like being explicit in this k6 new command’s output and including VUs and iterations as outputted examples. Sure, we can also mention VUs and iterations in k6-docs here and there, but in general (all k6-docs, if that’s what we mean), I don’t believe we should add more clutter to that already crowded browser options block.

options: {
browser: {
type: "chromium",
},
},
},
}, {{ if .EnableCloud }}
cloud: { {{ if .ProjectID }}
projectID: {{ .ProjectID }}, {{ else }}
// projectID: 12345, // Replace this with your own projectID {{ end }}
name: "{{ .ScriptName }}",
}, {{ end }}
};

export function setup() {
let res = http.get(BASE_URL);
if (res.status !== 200) {
throw new Error(
`Got unexpected status code ${res.status} when trying to setup. Exiting.`
);
}
}

export default async function() {
oleiade marked this conversation as resolved.
Show resolved Hide resolved
let checkData;
const page = await browser.newPage();
try {
Copy link
Contributor

@joanlopez joanlopez Jan 13, 2025

Choose a reason for hiding this comment

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

Would adding a catch here be a recommended practice? 🤔
If so, I'd prefer to have it in place, as this not being a simple test, but likely an entrypoint for k6 newcomers.

Copy link
Member

Choose a reason for hiding this comment

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

I believe in this case, if the error is not caught, the iteration will be aborted with an error anyway. But it would make sense indeed, if possible to catch the error in order to maybe explicitly call fail or exec.test.abort with a custom message guiding the user through what failed, why, and what they can do about it 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added catch + fail 👀

Also, another thing we can maybe use in all examples cc @ankur22

Copy link
Member Author

Choose a reason for hiding this comment

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

I have also changed the setup() of the browser and protocol tests to use exec.test.abort instead of throw

await page.goto(BASE_URL);
checkData = await page.locator("h1").textContent();
check(page, {
header: checkData === "Looking to break out of your pizza routine?",
});
await page.locator('//button[. = "Pizza, Please!"]').click();
await page.waitForTimeout(500);
await page.screenshot({ path: "screenshot.png" });
checkData = await page.locator("div#recommendations").textContent();
check(page, {
recommendation: checkData !== "",
});
} finally {
await page.close();
}
sleep(1);
}
18 changes: 18 additions & 0 deletions cmd/newtemplates/minimal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import http from 'k6/http';
import { sleep, check } from 'k6';

export const options = {
vus: 10,
duration: '30s',{{ if .EnableCloud }}
cloud: { {{ if .ProjectID }}
projectID: {{ .ProjectID }}, {{ else }}
// projectID: 12345, // Replace this with your own projectID {{ end }}
name: "{{ .ScriptName }}",
}, {{ end }}
};

export default function() {
let res = http.get('https://quickpizza.grafana.com');
check(res, { "status is 200": (res) => res.status === 200 });
sleep(1);
}
52 changes: 52 additions & 0 deletions cmd/newtemplates/protocol.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import http from "k6/http";
import { check, sleep } from "k6";

const BASE_URL = __ENV.BASE_URL || 'https://quickpizza.grafana.com';

export const options = {
vus: 10,
stages: [
{ duration: "10s", target: 5 },
{ duration: "20s", target: 10 },
{ duration: "1s", target: 0 },
], thresholds: {
http_req_failed: ["rate<0.01"],
http_req_duration: ["p(95)<500", "p(99)<1000"],
},{{ if .EnableCloud }}
cloud: { {{ if .ProjectID }}
projectID: {{ .ProjectID }}, {{ else }}
// projectID: 12345, // Replace this with your own projectID {{ end }}
name: "{{ .ScriptName }}",
}, {{ end }}
};

export function setup() {
let res = http.get(BASE_URL);
if (res.status !== 200) {
throw new Error(
`Got unexpected status code ${res.status} when trying to setup. Exiting.`
);
}
}

export default function() {
let restrictions = {
maxCaloriesPerSlice: 500,
mustBeVegetarian: false,
excludedIngredients: ["pepperoni"],
excludedTools: ["knife"],
maxNumberOfToppings: 6,
minNumberOfToppings: 2
};

let res = http.post(BASE_URL + "/api/pizza", JSON.stringify(restrictions), {
headers: {
'Content-Type': 'application/json',
'X-User-ID': 23423,
dgzlopes marked this conversation as resolved.
Show resolved Hide resolved
},
});

check(res, { "status is 200": (res) => res.status === 200 });
console.log(res.json().pizza.name + " (" + res.json().pizza.ingredients.length + " ingredients)");
sleep(1);
}
Loading
Loading