Test scaffolding feature

[kingo55]

I think I accidentally started a review - just meant to add comments on the code.

Anyway, I've been trying it out here on a local Mojito directory and it's looking quite good. Just noticed some formatting inconsistencies mostly.

[kingo55]

I'm beginning to think new is a more sensible command name than scaffold:

➜  med git:(development) ✗ gulp new --aa aa1
[11:37:24] Using gulpfile ~/Documents/med/gulpfile.js
[11:37:24] Starting 'new'...
[11:37:24] Finished 'new' after 2.94 ms

Feels like new is simpler and more intuitive, but scaffold is more descriptive.

[kingo55]

Something else I've been thinking about - Do we want to enforce the pattern of pushing live to 100% of traffic? Or should we encourage pushing live at 10%? Other split testing tools in the market push all experiments live at 100% by default, so it's probably the expected behaviour.

It raises the question of whether we allow a canary command, that releases to just sampleRate: 0.1.

[allmywant]

@kingo55 I agree with you that new more intuitive.

Do we want to enforce the pattern of pushing live to 100% of traffic?

I enforced pushing live to 100% of traffic in the code, but if we need we can change it by adding a extra flag: gulp set --live {{wave id}} --traffic {{simple rate}}, if traffic isn't specified then 100% traffic.

[kingo55]

@allmywant - yes, I think --traffic is a good idea for a new flag to manage launches. This will be for --live only, right?

As for new, let's use that instead of scaffold, because it's more intuitive.

Please add this functionality.

[kingo55]

@allmywant, is there a better way to organise ./cli-scaffold and ./cli-set files other than in the root of the repo? If we add more functionality like automated unit testing and screenshotting for experiments, then this folder could start looking crowded.

No big deal if not.

[allmywant]

@allmywant - yes, I think --traffic is a good idea for a new flag to manage launches. This will be for --live only, right?

As for new, let's use that instead of scaffold, because it's more intuitive.

Please add this functionality.

Yes, traffic will be for --live only, and it's optional, default value is 100%.

[allmywant]

@allmywant, is there a better way to organise ./cli-scaffold and ./cli-set files other than in the root of the repo? If we add more functionality like automated unit testing and screenshotting for experiments, then this folder could start looking crowded.

No big deal if not.

How about create a cli folder and put all cli command files in it?

[kingo55]

How about create a cli folder and put all cli command files in it?

Sure, that might work. What about all the helper modules?

[allmywant]

How about create a cli folder and put all cli command files in it?

Sure, that might work. What about all the helper modules?

OK, then we might need a better folder name than cli, how about helpers?

[allmywant]

@kingo55

Sure, that might work. What about all the helper modules?

How about create a scripts folder and put all cli and helper files in it?

[kingo55]

How about create a scripts folder and put all cli and helper files in it?

Yes, I think that sounds like a better name. If you can make that change, let's get this merged in.

[allmywant]

Yes, I think that sounds like a better name. If you can make that change, let's get this merged in.

Done.

[kingo55]

Sorry @allmywant - now that I'm trying it out in the CLI here, I think we need to make another small change. Typing new --new feels unnatural.

Perhaps we should change the --new flag to --wave? e.g.: gulp new --wave w232

[dapperdrop]

Sorry @allmywant - now that I'm trying it out in the CLI here, I think we need to make another small change. Typing new --new feels unnatural.

Perhaps we should change the --new flag to --wave? e.g.: gulp new --wave w232

@kingo55 @allmywant

Should we just drop the --new flag altogether?

E.g. gulp new {{wave id}} just creates a new test. If we add the --aa or --demo flags, it creates those test types instead

[allmywant]

Sorry @allmywant - now that I'm trying it out in the CLI here, I think we need to make another small change. Typing new --new feels unnatural. Perhaps we should change the --new flag to --wave? e.g.: gulp new --wave w232

@kingo55 @allmywant

Should we just drop the --new flag altogether?

E.g. gulp new {{wave id}} just creates a new test. If we add the --aa or --demo flags, it creates those test types instead

@dapperdrop We can't drop the --new flag altogether because of the problem I mentioned in Trello (https://trello.com/c/HKo0wz2O/454-mojito-build-script-test-scaffolding-feature#comment-5db3e2259096eb04c013936f)

gulp new {{wave id}}, in this case, gulp recognizes new as a gulp task, so it throws an error Task never defined: new. So we have to specify a flag, --new, --wave or something else.

[dapperdrop]

Sorry @allmywant - now that I'm trying it out in the CLI here, I think we need to make another small change. Typing new --new feels unnatural. Perhaps we should change the --new flag to --wave? e.g.: gulp new --wave w232

@kingo55 @allmywant Should we just drop the --new flag altogether? E.g. gulp new {{wave id}} just creates a new test. If we add the --aa or --demo flags, it creates those test types instead

@dapperdrop We can't drop the --new flag altogether because of the problem I mentioned in Trello (https://trello.com/c/HKo0wz2O/454-mojito-build-script-test-scaffolding-feature#comment-5db3e2259096eb04c013936f)

gulp new {{wave id}}, in this case, gulp recognizes new as a gulp task, so it throws an error Task never defined: new. So we have to specify a flag, --new, --wave or something else.

Good point. I think maybe we should consider changing the command to something like gulp create so we can do away with the --new flag. @kingo55 @allmywant

[kingo55]

@dapperdrop - based on the Gulp issue, I don't think we can drop --new altogether because it treats the next argument as a command:

➜  xxx git:(master) gulp scripts w122
[15:28:55] Using gulpfile ~/Documents/mojito-pipelines/xxx/gulpfile.js
[15:28:55] Task never defined: w122
[15:28:55] To list available tasks, try running: gulp --tasks

@allmywant - is this correct?

[allmywant]

@dapperdrop - based on the Gulp issue, I don't think we can drop --new altogether because it treats the next argument as a command:

➜  xxx git:(master) gulp scripts w122
[15:28:55] Using gulpfile ~/Documents/mojito-pipelines/xxx/gulpfile.js
[15:28:55] Task never defined: w122
[15:28:55] To list available tasks, try running: gulp --tasks

@allmywant - is this correct?

@kingo55 Yes, it is. Now we can change the command like @dapperdrop suggested or the flag.

[dapperdrop]

@allmywant @kingo55 I'm a little confused by

based on the Gulp issue, I don't think we can drop --new altogether because it treats the next argument as a command:

Looking through the gulpfile, it seems that arguments are handled by the logic in getCLIargs() and the reason that gulp scripts w122 doesn't work is that the scripts() function doesn't use getCLIargs().

So @allmywant should be able to customise the logic to drop --new from gulp create (if we go with this naming convention).

[allmywant]

@allmywant @kingo55 I'm a little confused by

based on the Gulp issue, I don't think we can drop --new altogether because it treats the next argument as a command:

Looking through the gulpfile, it seems that arguments are handled by the logic in getCLIargs() and the reason that gulp scripts w122 doesn't work is that the scripts() function doesn't use getCLIargs().

So @allmywant should be able to customise the logic to drop --new from gulp create (if we go with this naming convention).

The gulp cli handles the arguments first, and gulp supports running multi tasks with a single command, e.g. gulp scripts test, gulp run scripts task first, then test.

So with the gulp scripts w122 command, gulp will take w122 as the next task to run. Since we don't have a w122 task, gulp will throw a Task never defined: w122. So we can't drop the new flag.

[dapperdrop]

Understood! Thanks @allmywant

So I think gulp new --id {{id}} makes sense. --wave is id nomenclature specific to Mint Metrics I think. cc @kingo55

[kingo55]

@dapperdrop When I suggested --wave {{id}} I was trying to differentiate between:

• --demo {{id}}
• --aa {{id}}

Perhaps we could use --test {{id}} which is more Mint Metrics agnostic then?

[dapperdrop]

@kingo55 I see.

For consistency maybe we can go with:

• --ab (regular split test type)
• --aa
• --demo

[kingo55]

Perfect... Let's go with --ab!

@allmywant - are you able to make this update please? Thanks for bearing with us while we work out this final piece...

[allmywant]

@kingo55 Yes, I will work on this.

[allmywant]

Perfect... Let's go with --ab!

@allmywant - are you able to make this update please? Thanks for bearing with us while we work out this final piece...

Done.

[kingo55]

Awesome... this is all working perfectly here, @allmywant !

[dapperdrop]

Excellent @allmywant!