fix(cli): fix json format handling

[BioPhoton]

I added more infos to the issue: #316 also #358 should go before

This PR includes:

• adopting model
• adjusting tests

Overview of the target architecture can be found here: https://github.com/code-pushup/cli/wiki/Research:-Module-architecture-and-responsibilities

closes #316

[Tlacenka]

I do not understand the changes being made to the tests. They often reintroduce the bad practices again or test logic in the wrong place (cli and core are after test refactor). I provided context in the comments below.

[Tlacenka]

This was removed on purpose for better readability of the tests. By having explicit values present, it is clear which arguments are passed to the test without having to scroll up and look at additional function definitions.

Suggested change

	const cli = (options = {}) =>
	yargsCli(
	objectToCliArgs({
	_: 'autorun',
	verbose: true,
	config: '/test/code-pushup.config.ts',
	'persist.outputDir': '/test',
	...options,
	}),
	{
	...DEFAULT_CLI_CONFIGURATION,
	commands: [yargsAutorunCommandObject()],
	},
	);

[BioPhoton]

they are always the same

[matejchalk]

It's not a huge issue, but a recurring problem, so maybe it helps to explain the motivation better.

In general, it's better when a test contains all the context you need to know regarding what inputs are provided. Extracting setup code goes against this principle. There's a balance to be struck, of course, e.g. when the setup is long and often repeated in different tests, then extracting it may be the better option.

The old version of this test is more understandable for me, because within the it-block I see everything I need to understand it - e.g. "Why do the assertions expect /test/code-pushup.config.ts? Ah yes, because that's what argument was passed in, I see that a few lines above.". But with the new version, the relevant inputs and outputs aren't colocated, so I have to scroll up to understand the whole picture.

In fact, the new cli function is only used in this one test, so there's no advantage with regards to reducing duplication. And I also don't understand why persist.filename is specified here, but e.g. persist.outputDir or config aren't, they seem to be just as relevant to the assertions.

holidays

[matejchalk]

This test scenario runs with --persist.format=stdout, that should be removed (no longer a valid option). And then I believe the assertions should stay the same, because JSON should be generated by default.

[matejchalk]

Suggested change

	format: [
	...new Set([...options.persist.format, 'json']),
	] as AutorunOptions['persist']['format'],
	format: [
	...new Set([...options.persist.format, 'json'] satisfies Format[]),
	],

or:

Suggested change

	format: [
	...new Set([...options.persist.format, 'json']),
	] as AutorunOptions['persist']['format'],
	format: [...new Set([...options.persist.format, 'json' as const])],

[matejchalk]

It's not a huge issue, but a recurring problem, so maybe it helps to explain the motivation better.

In general, it's better when a test contains all the context you need to know regarding what inputs are provided. Extracting setup code goes against this principle. There's a balance to be struck, of course, e.g. when the setup is long and often repeated in different tests, then extracting it may be the better option.

The old version of this test is more understandable for me, because within the it-block I see everything I need to understand it - e.g. "Why do the assertions expect /test/code-pushup.config.ts? Ah yes, because that's what argument was passed in, I see that a few lines above.". But with the new version, the relevant inputs and outputs aren't colocated, so I have to scroll up to understand the whole picture.

In fact, the new cli function is only used in this one test, so there's no advantage with regards to reducing duplication. And I also don't understand why persist.filename is specified here, but e.g. persist.outputDir or config aren't, they seem to be just as relevant to the assertions.