Refactor new command

[dgzlopes]

Early draft PR. Issue with more context: #4154

What?

Why?

Checklist

• I have performed a self-review of my code.
• I have added tests for my changes.
• I have run linter locally (make 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)

[dgzlopes]

Happy to fix the linter things if we feel this approach makes sense 👍

[oleiade]

Looks good to me 👍🏻 🚀
Honestly, I'd be happy for us to move forward with this. Have you considered moving back to an engineering career Daniel ? 🙉

[joanlopez]

Great job @dgzlopes! 🚀

What do you think about adding code comments on every relevant line present on each template? I know that's very common nowadays across frameworks or similar tools that offer features for setting up a skeleton of your project. In fact, that's how our static template looks like now, and I think it's a nice-to-keep.

Especially considering that, I guess, the main target of this feature is new users of k6 - I suspect experienced users have their own scripts they duplicate and adjust.

[joanlopez]

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?

[oleiade]

👍🏻 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?

[dgzlopes]

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

[ankur22]

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

[inancgumus]

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.

[joanlopez]

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.

[oleiade]

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 👍🏻

[dgzlopes]

I have added catch + fail 👀

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

[dgzlopes]

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

[joanlopez]

Nit; I guess we could define the available "types" as exposed constants from the package, so it's a bit more explicit. But feel free to disregard this comment, cause for now it's only used by new, and very concretely.

[oleiade]

I'd pretty much like that too 👍🏻 Having them as constants, would help defining them as keys we explicitly own (and also help with later refactorings).

[dgzlopes]

Makes sense. Done!

[joanlopez]

I'd try to avoid the use of panic here, but return whatever error happened (or the result of joining both if more than one happened). I think errors are handled (likely by calling panic as well) in a "centralized" place, so I think it's better if we can just return, from here.

[dgzlopes]

Gotcha! I removed the panic 👀

[dgzlopes]

What do you think about adding code comments on every relevant line present on each template? I know that's very common nowadays across frameworks or similar tools that offer features for setting up a skeleton of your project. In fact, that's how our static template looks like now, and I think it's a nice-to-keep.

Especially considering that, I guess, the main target of this feature is new users of k6 - I suspect experienced users have their own scripts they duplicate and adjust.

@joanlopez My feeling is that the scripts are small/simple enough to be understandable out of the box. Also, after checking our docs, I saw that we have a weird mix of commented/un-commented examples.

With that said, I have no strong opinions on the topic, so yeah, happy to add comments if you think it would provide a better experience for newcomers 💪

[joanlopez]

@joanlopez My feeling is that the scripts are small/simple enough to be understandable out of the box. Also, after checking our docs, I saw that we have a weird mix of commented/un-commented examples.

With that said, I have no strong opinions on the topic, so yeah, happy to add comments if you think it would provide a better experience for newcomers 💪

If you ask me, I prefer it with comments, as we have now. But I don't think it's (only) me who should take that decision.
So, approving for now -as all the other comments are resolved now I think- we can always add the comments later if that's what we decided as a group consensus.