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.

Sign up for GitHub

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

More cmd/ refactoring to simplify test loading and support integration tests #2412

Merged
merged 10 commits into from
Mar 18, 2022
Merged

More cmd/ refactoring to simplify test loading and support integration tests #2412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8e8da96
Refactor test loading by moving it to a new helper function
na-- Mar 6, 2022
66d2f1f
Refactor abort signal handling into a separate helper
na-- Mar 6, 2022
37ade41
Move K6_TYPE / --type / -t out of the globalFlags to RuntimeOptions
na-- Mar 6, 2022
7671dc7
Simplify createOutputs()
na-- Mar 6, 2022
b1c270d
Refactor cmd.rootCommand to support easy integration tests
na-- Mar 7, 2022
0581cc4
Fix --log-output=file support for relative paths and add a test
na-- Mar 7, 2022
3b3e386
Rename and document better the cmd/inspect.go internals
na-- Mar 7, 2022
85c5169
Remove js.Bundle.IsExecutable() because it's not needed anymore
na-- Mar 7, 2022
1455a62
Move some output init calls away from the Engine
na-- Mar 8, 2022
6ffdb95
Improve some code identifier names and comments
na-- Mar 16, 2022
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
60 changes: 10 additions & 50 deletions cmd/archive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,9 @@ package cmd
import (
"github.com/spf13/cobra"
"github.com/spf13/pflag"

"go.k6.io/k6/errext"
"go.k6.io/k6/errext/exitcodes"
"go.k6.io/k6/lib/metrics"
)

func getArchiveCmd(globalState *globalState) *cobra.Command { // nolint: funlen
func getArchiveCmd(gs *globalState) *cobra.Command {
archiveOut := "archive.tar"
// archiveCmd represents the archive command
archiveCmd := &cobra.Command{
Expand All @@ -46,60 +42,24 @@ An archive is a fully self-contained test run, and can be executed identically e
k6 run myarchive.tar`[1:],
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
src, filesystems, err := readSource(globalState, args[0])
if err != nil {
return err
}

runtimeOptions, err := getRuntimeOptions(cmd.Flags(), globalState.envVars)
if err != nil {
return err
}

registry := metrics.NewRegistry()
builtinMetrics := metrics.RegisterBuiltinMetrics(registry)
r, err := newRunner(
globalState.logger, src, globalState.flags.runType,
filesystems, runtimeOptions, builtinMetrics, registry,
)
if err != nil {
return err
}

cliOpts, err := getOptions(cmd.Flags())
if err != nil {
return err
}
conf, err := getConsolidatedConfig(globalState, Config{Options: cliOpts}, r.GetOptions())
if err != nil {
return err
}

// Parse the thresholds, only if the --no-threshold flag is not set.
// If parsing the threshold expressions failed, consider it as an
// invalid configuration error.
if !runtimeOptions.NoThresholds.Bool {
for _, thresholds := range conf.Options.Thresholds {
err = thresholds.Parse()
if err != nil {
return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
}
}
}

_, err = deriveAndValidateConfig(conf, r.IsExecutable, globalState.logger)
test, err := loadAndConfigureTest(gs, cmd, args, getPartialConfig)
if err != nil {
return err
}

err = r.SetOptions(conf.Options)
// It's important to NOT set the derived options back to the runner
// here, only the consolidated ones. Otherwise, if the script used
// an execution shortcut option (e.g. `iterations` or `duration`),
// we will have multiple conflicting execution options since the
// derivation will set `scenarios` as well.
err = test.initRunner.SetOptions(test.consolidatedConfig.Options)
if err != nil {
return err
}

// Archive.
arc := r.MakeArchive()
f, err := globalState.fs.Create(archiveOut)
arc := test.initRunner.MakeArchive()
f, err := gs.fs.Create(archiveOut)
if err != nil {
return err
}
Expand Down
17 changes: 2 additions & 15 deletions cmd/archive_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ import (
"testing"

"github.com/spf13/afero"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.k6.io/k6/errext"
"go.k6.io/k6/errext/exitcodes"
)

Expand Down Expand Up @@ -51,21 +49,10 @@ func TestArchiveThresholds(t *testing.T) {
testState.args = append(testState.args, "--no-thresholds")
}

gotErr := newRootCommand(testState.globalState).cmd.Execute()

assert.Equal(t,
testCase.wantErr,
gotErr != nil,
"archive command error = %v, wantErr %v", gotErr, testCase.wantErr,
)

if testCase.wantErr {
var gotErrExt errext.HasExitCode
require.ErrorAs(t, gotErr, &gotErrExt)
assert.Equalf(t, exitcodes.InvalidConfig, gotErrExt.ExitCode(),
"status code must be %d", exitcodes.InvalidConfig,
)
testState.expectedExitCode = int(exitcodes.InvalidConfig)
}
newRootCommand(testState.globalState).execute()
})
}
}
89 changes: 25 additions & 64 deletions cmd/cloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import (
"path/filepath"
"strconv"
"sync"
"syscall"
"time"

"github.com/fatih/color"
Expand All @@ -42,7 +41,6 @@ import (
"go.k6.io/k6/errext/exitcodes"
"go.k6.io/k6/lib"
"go.k6.io/k6/lib/consts"
"go.k6.io/k6/lib/metrics"
"go.k6.io/k6/ui/pb"
)

Expand Down Expand Up @@ -91,56 +89,23 @@ This will execute the test on the k6 cloud service. Use "k6 login cloud" to auth
RunE: func(cmd *cobra.Command, args []string) error {
printBanner(globalState)

logger := globalState.logger
progressBar := pb.New(
pb.WithConstLeft("Init"),
pb.WithConstProgress(0, "Parsing script"),
pb.WithConstProgress(0, "Loading test script..."),
)
printBar(globalState, progressBar)

// Runner
filename := args[0]
src, filesystems, err := readSource(globalState, filename)
if err != nil {
return err
}

runtimeOptions, err := getRuntimeOptions(cmd.Flags(), globalState.envVars)
if err != nil {
return err
}

modifyAndPrintBar(globalState, progressBar, pb.WithConstProgress(0, "Getting script options"))
registry := metrics.NewRegistry()
builtinMetrics := metrics.RegisterBuiltinMetrics(registry)
r, err := newRunner(logger, src, globalState.flags.runType, filesystems, runtimeOptions, builtinMetrics, registry)
if err != nil {
return err
}

modifyAndPrintBar(globalState, progressBar, pb.WithConstProgress(0, "Consolidating options"))
cliOpts, err := getOptions(cmd.Flags())
if err != nil {
return err
}
conf, err := getConsolidatedConfig(globalState, Config{Options: cliOpts}, r.GetOptions())
test, err := loadAndConfigureTest(globalState, cmd, args, getPartialConfig)
if err != nil {
return err
}

// Parse the thresholds, only if the --no-threshold flag is not set.
// If parsing the threshold expressions failed, consider it as an
// invalid configuration error.
if !runtimeOptions.NoThresholds.Bool {
for _, thresholds := range conf.Options.Thresholds {
err = thresholds.Parse()
if err != nil {
return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
}
}
}

derivedConf, err := deriveAndValidateConfig(conf, r.IsExecutable, logger)
// It's important to NOT set the derived options back to the runner
// here, only the consolidated ones. Otherwise, if the script used
// an execution shortcut option (e.g. `iterations` or `duration`),
// we will have multiple conflicting execution options since the
// derivation will set `scenarios` as well.
err = test.initRunner.SetOptions(test.consolidatedConfig.Options)
if err != nil {
return err
}
Expand All @@ -149,13 +114,9 @@ This will execute the test on the k6 cloud service. Use "k6 login cloud" to auth
// TODO: validate for externally controlled executor (i.e. executors that aren't distributable)
// TODO: move those validations to a separate function and reuse validateConfig()?

err = r.SetOptions(conf.Options)
if err != nil {
return err
}
modifyAndPrintBar(globalState, progressBar, pb.WithConstProgress(0, "Building the archive..."))
arc := test.initRunner.MakeArchive()

modifyAndPrintBar(globalState, progressBar, pb.WithConstProgress(0, "Building the archive"))
arc := r.MakeArchive()
// TODO: Fix this
// We reuse cloud.Config for parsing options.ext.loadimpact, but this probably shouldn't be
// done, as the idea of options.ext is that they are extensible without touching k6. But in
Expand All @@ -174,7 +135,7 @@ This will execute the test on the k6 cloud service. Use "k6 login cloud" to auth

// Cloud config
cloudConfig, err := cloudapi.GetConsolidatedConfig(
derivedConf.Collectors["cloud"], globalState.envVars, "", arc.Options.External)
test.derivedConfig.Collectors["cloud"], globalState.envVars, "", arc.Options.External)
if err != nil {
return err
}
Expand Down Expand Up @@ -205,12 +166,14 @@ This will execute the test on the k6 cloud service. Use "k6 login cloud" to auth

name := cloudConfig.Name.String
if !cloudConfig.Name.Valid || cloudConfig.Name.String == "" {
name = filepath.Base(filename)
name = filepath.Base(test.sourceRootPath)
}

globalCtx, globalCancel := context.WithCancel(globalState.ctx)
defer globalCancel()

logger := globalState.logger

// Start cloud test run
modifyAndPrintBar(globalState, progressBar, pb.WithConstProgress(0, "Validating script options"))
client := cloudapi.NewClient(
Expand All @@ -226,13 +189,10 @@ This will execute the test on the k6 cloud service. Use "k6 login cloud" to auth
}

// Trap Interrupts, SIGINTs and SIGTERMs.
sigC := make(chan os.Signal, 2)
globalState.signalNotify(sigC, os.Interrupt, syscall.SIGINT, syscall.SIGTERM)
defer globalState.signalStop(sigC)
go func() {
sig := <-sigC
gracefulStop := func(sig os.Signal) {
logger.WithField("sig", sig).Print("Stopping cloud test run in response to signal...")
// Do this in a separate goroutine so that if it blocks the second signal can stop the execution
// Do this in a separate goroutine so that if it blocks, the
// second signal can still abort the process execution.
go func() {
stopErr := client.StopCloudTestRun(refID)
if stopErr != nil {
Expand All @@ -242,20 +202,21 @@ This will execute the test on the k6 cloud service. Use "k6 login cloud" to auth
}
globalCancel()
}()

sig = <-sigC
}
hardStop := func(sig os.Signal) {
logger.WithField("sig", sig).Error("Aborting k6 in response to signal, we won't wait for the test to end.")
os.Exit(int(exitcodes.ExternalAbort))
}()
Comment on lines +206 to -249
Copy link
Contributor

Choose a reason for hiding this comment

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

The names here are not ... good IMO.

I was really confused that gracefulStop does something while hardStop just logs and does not os.Exit.

I would argue they are not great in the handleTestAbortSignals either.

I would recommend adding on in front the names in all places. This still isn't great, but I feel a lot better about onHardStop not calling os.Exit explicitly and only logging, while onGracefulStop actually doing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe just have os.Exit in both hardStops 🤷 I kind of feel like this DRY-ing of the code isn't really worth 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.

👍 for gracefulStop and onHardStop
Regarding the DRY, keep in mind that this will be used at least 2 more times, in k6 coordinate and k6 agent

Copy link
Member Author

Choose a reason for hiding this comment

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

(for expediency, I will address this in the next PR)

}
stopSignalHandling := handleTestAbortSignals(globalState, gracefulStop, hardStop)
defer stopSignalHandling()

et, err := lib.NewExecutionTuple(derivedConf.ExecutionSegment, derivedConf.ExecutionSegmentSequence)
et, err := lib.NewExecutionTuple(test.derivedConfig.ExecutionSegment, test.derivedConfig.ExecutionSegmentSequence)
if err != nil {
return err
}
testURL := cloudapi.URLForResults(refID, cloudConfig)
executionPlan := derivedConf.Scenarios.GetFullExecutionRequirements(et)
executionPlan := test.derivedConfig.Scenarios.GetFullExecutionRequirements(et)
printExecutionDescription(
globalState, "cloud", filename, testURL, derivedConf, et, executionPlan, nil,
globalState, "cloud", test.sourceRootPath, testURL, test.derivedConfig, et, executionPlan, nil,
)

modifyAndPrintBar(
Expand Down
58 changes: 35 additions & 23 deletions cmd/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,16 @@
package cmd

import (
"archive/tar"
"bytes"
"fmt"
"os"
"syscall"

"github.com/spf13/afero"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
"gopkg.in/guregu/null.v3"

"go.k6.io/k6/errext/exitcodes"
"go.k6.io/k6/lib/types"
"go.k6.io/k6/loader"
)

// Panic if the given error is not nil.
Expand Down Expand Up @@ -86,28 +85,41 @@ func exactArgsWithMsg(n int, msg string) cobra.PositionalArgs {
}
}

// readSource is a small wrapper around loader.ReadSource returning
// result of the load and filesystems map
func readSource(globalState *globalState, filename string) (*loader.SourceData, map[string]afero.Fs, error) {
pwd, err := globalState.getwd()
if err != nil {
return nil, nil, err
func printToStdout(gs *globalState, s string) {
if _, err := fmt.Fprint(gs.stdOut, s); err != nil {
gs.logger.Errorf("could not print '%s' to stdout: %s", s, err.Error())
}

filesystems := loader.CreateFilesystems(globalState.fs)
src, err := loader.ReadSource(globalState.logger, filename, pwd, filesystems, globalState.stdIn)
return src, filesystems, err
}

func detectType(data []byte) string {
if _, err := tar.NewReader(bytes.NewReader(data)).Next(); err == nil {
return typeArchive
}
return typeJS
}
// Trap Interrupts, SIGINTs and SIGTERMs and call the given.
func handleTestAbortSignals(gs *globalState, firstHandler, secondHandler func(os.Signal)) (stop func()) {
sigC := make(chan os.Signal, 2)
done := make(chan struct{})
gs.signalNotify(sigC, os.Interrupt, syscall.SIGINT, syscall.SIGTERM)

func printToStdout(gs *globalState, s string) {
if _, err := fmt.Fprint(gs.stdOut, s); err != nil {
gs.logger.Errorf("could not print '%s' to stdout: %s", s, err.Error())
go func() {
select {
case sig := <-sigC:
firstHandler(sig)
case <-done:
return
}

select {
case sig := <-sigC:
if secondHandler != nil {
secondHandler(sig)
}
Comment on lines +110 to +112
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is this nil check needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think so, if os.Exit() is here. And I kind of like it to be here, to softly force the user to acknowledge that a second Ctrl+C should always abort the execution almost immediately, not be something optional.

// If we get a second signal, we immediately exit, so something like
// https://github.com/k6io/k6/issues/971 never happens again
gs.osExit(int(exitcodes.ExternalAbort))
case <-done:
return
}
}()

return func() {
close(done)
gs.signalStop(sigC)
}
}
10 changes: 10 additions & 0 deletions cmd/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,16 @@ func (c Config) Apply(cfg Config) Config {
return c
}

// Returns a Config but only parses the Options inside.
func getPartialConfig(flags *pflag.FlagSet) (Config, error) {
opts, err := getOptions(flags)
if err != nil {
return Config{}, err
}

return Config{Options: opts}, nil
}

// Gets configuration from CLI flags.
func getConfig(flags *pflag.FlagSet) (Config, error) {
opts, err := getOptions(flags)
Expand Down
Loading