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

Consider adding configuration in reporter declaration #43

Closed
pascalduez opened this issue Jun 12, 2018 · 9 comments
Closed

Consider adding configuration in reporter declaration #43

pascalduez opened this issue Jun 12, 2018 · 9 comments
Assignees

Comments

@pascalduez
Copy link
Contributor

hi,

We can already use the plugin from a jest.config.js as such:

reporters: [
  'default',
  'jest-html-reporter',
]

But we need to create a jesthtmlreporter.config.json.

Desired (fits other reporters config api):

reporters: [
  'default',
  [
    'jest-html-reporter',
    {
      executionMode: 'reporter',
      pageTitle: '...',
      outputPath: '...',
    },
  ],
],
@Hargne
Copy link
Owner

Hargne commented Jun 12, 2018

This is a pretty neat idea. Will try to look into it ASAP.

@jpicton
Copy link

jpicton commented Jun 15, 2018

I'll upvote this idea. Took me a while to understand why the reporters options format wasn't working for this package. It's a bit messy having to spread Jest-related configuration around in multiple files/directories.

@Hargne Hargne self-assigned this Jun 15, 2018
@Hargne Hargne added this to To do in New Features via automation Jun 15, 2018
@Hargne
Copy link
Owner

Hargne commented Jun 15, 2018

Hi,

I am currently working on this feature as I believed it to be a nice improvement of the plugin.
A small dilemma has appeared however in which I would like to get some inputs on in order to solve it as smoothly as possible:

As the plugin is designed at the moment, it expects to be run as a testResultsProcessor by default - meaning that it will read the config from jesthtmlreporter.config.json.
Running the plugin as a Custom Reporter is priority 2, meaning that you have to actively define inside jesthtmlreporter.config.json that you want it to be run as such. This would eventually mean that you would have to create a jesthtmlreporter.config.json-file in either case.

The reason the execution mode needs to be defined is because of how Jest requires different exports depending on these modes.

If we were to swap the priority order (Custom Reporter being default, and testResultsProcessor needing configuration), it would break the plugin for all users who haven't defined executionMode in jesthtmlreporter.config.json, and it would also mean that we would somewhat deviate from the pattern of keeping configurations separate (as discussed in this issue #20)

What are your thoughts on this matter?

@jpicton
Copy link

jpicton commented Jun 15, 2018

Can you do something similar to what they’ve done with jest-junit (which can also be used in both ways)? See https://github.com/jest-community/jest-junit/blob/master/index.js

@pascalduez
Copy link
Contributor Author

@Hargne Thanks for the explanation!
Actually the point of this request is being able to dismiss the jesthtmlreporter.config.json and having all the jest related configurations in one file. Mostly because it's easier to maintain across several packages.

Hargne added a commit that referenced this issue Jun 17, 2018
…porter declaration, Modified the default export to automatically handle if it was invoked as a custom reporter or test result processor, Removed executionMode configuration option, Updated README
@Hargne
Copy link
Owner

Hargne commented Jun 17, 2018

@pascalduez @jpicton could you take a look at the PR regarding this (#44) and see if this solves this issue for you?

Thanks!

@jpicton
Copy link

jpicton commented Jun 18, 2018

Cheers @Hargne, the changes look like they're exactly what I was after.

Hargne added a commit that referenced this issue Jun 18, 2018
@Hargne
Copy link
Owner

Hargne commented Jun 18, 2018

Added in #44

@Hargne Hargne closed this as completed Jun 18, 2018
New Features automation moved this from To do to Released Jun 18, 2018
@pascalduez
Copy link
Contributor Author

@Hargne Works like a charm, thanks a bunch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
New Features
  
Released
Development

No branches or pull requests

3 participants