[Improve] Suggestions for the code coverage options after the v2.0.0 refactor

1) Do we need the `coverage.reportPath` option ?

I know I mentioned that we should be able to choose which html report to display on the monocart test results reporter with #69.

However, given the awesome work you've done on the release v2.0.0 of the monocart-reporter package I was wondering if the coverage.reportPath option is even needed?

What I'm thinking is that you know all the possible reporters that will generate an html report, they are:

the v8 report
the istanbul reporters that produce an html report: html or html-spa (don't think there are others)

Since you know the reporters that can produce an html report then my suggestion would be to include them all by default in under the monocart report attachments global:

Other considerations:

Consider adding an option that enables selecting the display name for the html reports. Otherwise you'd have to choose some kind of default attachment name.
Consider adding an option that controls whether to attach or not an html report to the global attachments. Would default to true.

Example of a playwright.config.ts with this suggestion would look like:

// playwright.config.ts

export default defineConfig({
  ...
  reporter: [
    ...
    [
      'monocart-reporter',
      {
        name: 'monocart playwright test results report',
        ...
        coverage: {
          outputDir: './tests/test-results/code-coverage',
          // reportPath: ... // this option would be removed
          reports: [
            ['v8', {
              name: 'v8 code coverage',
              ...
            }],
            ['html', {
              name: 'istanbul html code coverage'.
              subdir: 'html-report',
            }],
            ['html-spa', {
              includeInGlobalAttachments: false,
              subdir: 'html-spa-report',
            }],
           ...
          ]
        }
      }
  ]
  ...
});

Note that in the above example the html-spa report wouldn't show under the global attachments of the monocart report because includeInGlobalAttachments is set to false but since includeInGlobalAttachments defaults to true, then the v8 and html reports would be included and named according to the name property.

Note: the names I'm suggesting are just that, suggestions, feel free to choose better names or a better structure for this issue.

2) Consolidate the `v8` report options under the `coverage.reports[v8]` section

What do you think about removing the coverage.outputFile and move that option inside the coverage.reports[v8] report options? That to me would make a bit more sense and keep it inline with the other reporters?

So that instead of the playwright.config.ts config shown in the comment above you would do:

// playwright.config.ts

export default defineConfig({
  testDir: './tests',
  outputDir: './tests/test-results'
  ...
  reporter: [
    ...
    [
      'monocart-reporter',
      {
        name: 'playwright code coverage with monocart reporter',
        outputFile:  './tests/test-results/monocart-report.html',
        coverage: {
          outputDir: './tests/test-results/code-coverage',
          reportPath: './tests/test-results/code-coverage/v8/index.html',
          reports: [
            ['v8', {
              file: 'v8/index.html' // Use this instead of coverage.outputFile
            }],
           ...
          ]
        }
      }
  ]
  ...
});

If you agree with that then perhaps the other v8 related options like inline and assetsPath could also be moved to options of the v8 report:

// [V8 only](Boolean) Inline all scripts to the single HTML file. Defaults to false.
inline?: boolean,

// [V8 only](String) Assets path if not inline. Defaults to "./assets"
assetsPath?: string

What are your thoughts?

3) Do we still need the `coverage.lcov` option?

Looking at monocart-coverage-reports/lib/index.d.ts, you've still kept the lcov option on the CoverageOptions.

// (Boolean) Generate lcov.info file, same as lcovonly report. Defaults to false.
lcov?: boolean,

Does it still make sense to keep it? With the current v2.0.0 release you can generate an lcov only report by specifying it in the reporters array as:

coverage: {
  reports: [
    ['lcovonly', {
      file: 'lcov/code-coverage.lcov.info',
    }],
  ]

Is there any reason to keep the coverage.lcov option? If there's no benefit to having this option then I think for a user it can sometimes be confusing to have competing options. And for you as a maintainer, if it's less code for you to maintain then great right?

1), the reportPath option is used to output a file link like "sub/index.html" or "some-name.json" which user could open it. by default, it will find a existing "outputDir/**/index.html" as the report link, also we can disable it with reportPath="". The display name is already supported with name option:

    // global coverage
    coverage: {
        name: 'My global coverage report name',
    },

2), there are 3 options for v8 report, you can pass it like istanbul reports

    // global coverage
    coverage: {
        name: 'My global coverage report name',
        reports: [
            ['v8', {
              outputFile: "index.html",
              inline: false,
              assetsPath: "./assets"
            }],
           ...
          ]
    },

I understand that for users, these parameter names are a little confused, and we hope their style is consistent including naming specifications. However, istanbul reports come from different system and the coding and naming style is different with monocart-reporter, we just integrate it and keep it's original style. For monocart-reporter and v8 coverage report, I don't want to do big changes to make the style look like istanbul's. For example the subdir, I prefer to subDir camelCase naming. Meanwhile, I want to keep same style between version 1.x and 2.x.

3), about lcov option, it is a shortcut

coverage: {
        reports: [
            ['v8'],
            ['lcovonly']
          ]
    },

equal to following but simpler

coverage: {
        reports: 'v8',
        lcov: true
    },

it lower priority if lcovonly exsits

coverage: {
        reports: [
            ['v8'],
            ['lcovonly', { ... }] // higher priority
          ],
          lcov: true // will be ignored if lcovonly exsits
    },

Also for v8 report options, the inside options is higher priority than outside if both exsits

coverage: {
        reports: [
            ['v8', { 
              // higher priority
              outputFile: "index.html",
              inline: false,
              assetsPath: "./assets"
            }]
          ],

          // lower priority will be ignored if inside options exsits
          outputFile: "index.html",
          inline: false,
          assetsPath: "./assets"'

    },

Sorry, I cannot provide more details in time, and the document needs to be improved. Welcome to discussions. thanks for your suggestions

1) Do we need the `coverage.reportPath` option ?

I understand your explanation and I can see how it is useful but I'm not sure I was able to explain myself properly.

My question/suggestion was that if you were to consider including all html reports by default as global attachments then you wouldn't need the coverage.reportPath option right? If you did this it would also mean users could configure a v8 report, and then a html and html-spa reporter and all 3 would be included in the global attachments instead of having to choose only one right?

That's why I showed a potential config for this where you could also then provide a name to control the display text for the global attachment and a includeInGlobalAttachments that could default to true but the users could set to false if they didn't want that html report included in the global coverage options.

Perhaps you understood this already from my initial explanation and you feel that it isn't worth the change?

2) Consolidate the v8 report options under the `coverage.reports[v8]` section

I didn't notice that the v8 report already had some available options. I only checked the https://github.com/cenfun/monocart-coverage-reports/blob/main/lib/index.d.ts to understand the available options and at the time it didn't have those options but you already updated it for v2.0.1 👏

I've tested the options you mentioned above and they work as expected.

I still think that the fact that you left the coverage.outputFile, coverage.inline and coverage.assetsPath is a bit confusing and for a user it's a better API if you don't have the same option in two different places. I understand that one set of options has higher priority than others but I still think removing these 3 in favour of leaving them in the v8 report options would be better.

3) Do we still need the `coverage.lcov` option?

Right, I understand that the coverage.lcov is a shortcut. For the same reasons explained for the point number two above, I believe it would be better for the user to NOT have two very similar ways of doing the same thing. I think that just leads to confusion and questions about what's the pros/cons on using one way versus the other, when for these options we're discussing, there's really no pros/cons that I can see, it's just "shortcuts".

Summary

All of these are feedback on simplifying the coverage options. I feel that having the "duplicate/shortcut" options like coverage.outputFile, coverage.inline, coverage.assetsPath and coverage.lcov hurt more than they help, specially for new users trying to learn how to use the coverage options.

I also think that the coverage.reportsPath is an improvement made to v2.0.0 but given all the other work you did to improve the coverage.reports I think it becomes a limiting option because it ONLY allows the user to select ONE html type report.

Lastly, I think that making these kind of changes in v2 is perfect. It's not a v1.X change which would be a breaking and potentially confusing change for users. Since it's a v2.X change then I think it's the perfect time to make these kind of changes if you think they're worth it.

[!Note]

All of the above points are my feedback as a user. Ultimately, if you feel it's better/ok to leave as is then it's fine, you have your vision for how things should work and nothing is broken so it's fine if you don't think it should change. Feel free to close this if you don't think it's worth doing any changes.

sorry I misunderstood. I think there is only one report could be shown as global report link, not all reports. and the best report format is html, maybe json is ok too, because html and json can be open directly in the browser. I don't think user need all coverage reports show in test report, It's enough to choose one favorite. I personally think that most of the time, what users need is v8+lcov or html+lcov or v8 only. so lets keep this unchanged until more feedback or new requirements thanks for your feedback.

Sounds good. I'll close this issue until there's more feedback or new requirements.

Thank you for your time @cenfun 👍

cenfun / monocart-reporter