Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support passing reporter options from config #459

Merged
merged 2 commits into from
Jan 11, 2024
Merged

feat: support passing reporter options from config #459

merged 2 commits into from
Jan 11, 2024

Conversation

jkowalleck
Copy link
Contributor

@jkowalleck jkowalleck commented Mar 22, 2023

I saw that the reporter allowed passing reporter options – since #423.
This PR originally also include a use case: c8 config.

{
  reporter: ['text'],
  reporterOptions: {
    text: { file: 'coverage.txt' },
  }
}

Here is another real world use case:

{
  "all": true,
  "reporter": ["text-summary", "clover", "html"],
  "reporterOptions": {
    "clover": {"file": "coverage.clover.xml"},
    "html": {"subdir": "coverage.html"}
  },
  "reports-dir": "./reports",
  "temp-directory": "./.c8.cache"
}

Unfortunately, the original feature forgot to implement the solution to the exact use case.
SO the actual feature was never accessible/working/existing.

Here is the solution: reporterOptions from the config are passed to reporters.

I do not have any idea how the CLI could look like to configure this. So I did not implement it and also did not update any docs.
CLI might be like c8 --reporter-option clover.file=coverage.clover.xml --reporter-option html.subdir=coverage.html. Or something else.

Checklist
  • npm test, tests passing
  • npm run test:snap (to update the snapshot)
  • tests and/or benchmarks are included
  • documentation is changed or added

@jkowalleck
Copy link
Contributor Author

rebased onto target branch, fixed conflicts, updated snap

@bcoe ping

@bcoe bcoe added the ci label Nov 21, 2023
@bcoe bcoe added ci and removed ci labels Jan 3, 2024
@@ -25,6 +25,7 @@ exports.outputReport = async function (argv) {
excludeAfterRemap: argv.excludeAfterRemap,
reporter: Array.isArray(argv.reporter) ? argv.reporter : [argv.reporter],
reportsDirectory: argv['reports-dir'],
reporterOptions: argv.reporterOptions || {},
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
reporterOptions: argv.reporterOptions || {},
reporterOptions: argv.reporterOptions ?? {},

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcoe i used the exiting methods. see

this.reporterOptions = reporterOptions || {}

@bcoe
Copy link
Owner

bcoe commented Jan 3, 2024

@jkowalleck apologies for ignoring this for so long, bother you to implement the suggestion.

@jkowalleck
Copy link
Contributor Author

@jkowalleck apologies for ignoring this for so long, bother you to implement the suggestion.

My current code reflects existing style.
https://github.com/bcoe/c8/pull/459/files#r1440834355

@bcoe bcoe added ci and removed ci labels Jan 4, 2024
@bcoe
Copy link
Owner

bcoe commented Jan 4, 2024

@jkowalleck your branch has conflicts, could I bother you to rebase and push.

@jkowalleck
Copy link
Contributor Author

will recreate the test snapshots during next week.

Signed-off-by: Jan Kowalleck <[email protected]>
@jkowalleck
Copy link
Contributor Author

@bcoe rebased onto main and recreated test snaps

Signed-off-by: Jan Kowalleck <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants