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

[na--]

More yak shaving, but after this, some parts of cmd/ are almost normal looking 😅 🎉 I particularly like how clean cmd/archive.go became 🎉

k6/cmd/archive.go

Lines 44 to 72 in 147e3d1

	RunE: func(cmd *cobra.Command, args []string) error {
	test, err := loadTest(gs, cmd, args, getPartialConfig)
	if err != nil {
	return err
	}
	// 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 := test.initRunner.MakeArchive()
	f, err := gs.fs.Create(archiveOut)
	if err != nil {
	return err
	}
	err = arc.Write(f)
	if cerr := f.Close(); err == nil && cerr != nil {
	err = cerr
	}
	return err
	},

Closes #342

[na--]

With the latest commit here, we can now write dead-simple and safe integration tests 🎉 See this for an example:

k6/cmd/integration_test.go

Lines 24 to 37 in 441a8f0

	func TestSimpleTestStdin(t *testing.T) {
	t.Parallel()
	ts := newGlobalTestState(t)
	ts.args = []string{"k6", "run", "-"}
	ts.stdIn = bytes.NewBufferString(noopDefaultFunc)
	newRootCommand(ts.globalState).execute()
	stdOut := ts.stdOut.String()
	assert.Contains(t, stdOut, "default: 1 iterations for each of 1 VUs")
	assert.Contains(t, stdOut, "1 complete and 0 interrupted iterations")
	assert.Empty(t, ts.stdErr.Bytes())
	assert.Empty(t, ts.loggerHook.Drain())
	}

So between this and the fact that we have xk6 integration tests in our CI, I think it's pretty safe to close #342 once this PR is merged 🎉

[na--]

And yeah, I consider these to be integration tests because this is what main.go does now:

k6/main.go

Lines 23 to 29 in 441a8f0

	import (
	"go.k6.io/k6/cmd"
	)
	func main() {
	cmd.Execute()
	}

k6/cmd/root.go

Lines 297 to 301 in 441a8f0

	func Execute() {
	gs := newGlobalState(context.Background())
	newRootCommand(gs).execute()
	}

And this is what an integration test in cmd/ can do:

k6/cmd/integration_test.go

Lines 27 to 30 in 441a8f0

	ts := newGlobalTestState(t)
	ts.args = []string{"k6", "run", "-"}
	ts.stdIn = bytes.NewBufferString(noopDefaultFunc)
	newRootCommand(ts.globalState).execute()

The two are basically the same, only the integration test has a dedicated environment (so we can run it safely in parallel) and also complete control that environment 🎉

[oleiade]

Thumbs up from me 👍 🎉 Kudos for those changes, I do believe they will make our lives easier indeed!

I do think it would be beneficial to find better names for some of the APIs introduced in this PR; see my comments smiley 🐱

[oleiade]

Besides my few comments on naming, looking good! The yak is shaved, and we're equipped to make ourselves a nice pull-over for the next winter. Kudos 🎉

[mstoykov]

Just runner I mean it seems to be the only runner either way.

[na--]

Maybe, but I'd prefer to wait with the final refactor and rename before messing with the names more...

[mstoykov]

LGTM in general. I have some naming comments that can be addressed in a separate PR

[mstoykov]

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.

[mstoykov]

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

[na--]

👍 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

[na--]

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

[mstoykov]

nit: is this nil check needed?

[na--]

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.

[mstoykov]

We do actually write this in the archive under type

https://github.com/grafana/k6/blob/master/js/bundle.go#L190

maybe one day we can make MakeArchive outside of a Runner 🤷