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

Use JSON report data instead of single-file html #26

Open
nicojs opened this issue May 18, 2020 · 10 comments
Open

Use JSON report data instead of single-file html #26

nicojs opened this issue May 18, 2020 · 10 comments

Comments

@nicojs
Copy link
Member

nicojs commented May 18, 2020

The mutationreport publisher right now works by publishing the HTML file and loading that in an IFrame (iframe in iframe), instead of publishing the JSON of the mutation testing report itself.

I have only communicate my dissatisfaction verbally, and did a poor job at it. Sorry for that. Let me try to convince you with arguments in this issue.

  • It only works for single-file reports.
    • Both Stryker and Stryker4s spread reports out over 3 files. Which means they have to change, or are not supported.
    • You force other frameworks which want to align on our mutation testing report JSON and HTML report to use a single file report. This is pretty hostile IMO. You might want to load other external resources on your html page, you now force all those resources to be embedded. Another scenario is that they might only want to support our json format, and are not be ready to support our HTML reporter. Too bad, you will not be supported.
    • It goes against the HTML specification and way of working.
    • We're forcing us to limit to 1 html file in the future. If we would ever want add features like lazy loading parts of the report, we're unable to do so without breaking the azure devops publisher.
  • It's a generic HTML file publisher.
    • This might sound like a lame excuse, but I like to call things what they are. Why not call it a html-file-publisher instead?
    • Since the publisher doesn't know anything about the mutation report, it cannot add logic later. Something like inline annotations comes to mind: (for example, how eslint does it in github: https://github.com/hallee/eslint-action/blob/master/screenshots/annotation.png), or add a summary of the mutation score somewhere. This won't be possible to add these kind of features if we don't have the data.
  • Maintainability.
    • I don't think loading an iframe in an iframe is great for maintainability. What if Microsoft decides to not allow iframes inside their iframe?
    • We're dependent on an external dependency (i.e. iframe resizer), so we need to maintain that dependency.
    • We're integrating 3 things (azure devops, mutation testing elements and the resizer) into one page, it's just a matter of time before this breaks IMO. Creating e2e tests for it which test the integration will help us with that, but those are also expensive to create and maintain.

My proposal is to create a new publisher that works with the report JSON and rebrand this one as a, more generic, html-file publisher.

@hugo-vrijswijk @richardwerkman @simondel @Mobrockers What are your thoughts?

@rouke-broersma
Copy link
Member

rouke-broersma commented May 18, 2020

The mutationreport publisher right now works by publishing the HTML file and loading that in an IFrame (iframe in iframe), instead of publishing the JSON of the mutation testing report itself.

I have only communicate my dissatisfaction verbally, and did a poor job at it. Sorry for that. Let me try to convince you with arguments in this issue.

  • It only works for single-file reports.

    • Both Stryker and Stryker4s spread reports out over 3 files. Which means they have to change, or are not supported.
    • You force other frameworks which want to align on our mutation testing report JSON and HTML report to use a single file report. This is pretty hostile IMO. You might want to load other external resources on your html page, you now force all those resources to be embedded. Another scenario is that they might only want to support our json format, and are not be ready to support our HTML reporter. Too bad, you will not be supported.

It's open source, they can adapt their solution, or our solution. Doesn't matter to me. If they need something else, contribute it :)
And actually, having a need/want to embed their custom html/css/js on top of the report is a perfect use case for a HTML report in the plugin, because if they don't then they only get our HTML report implementation ;) The situation you propose cannot be supported by this extension at all without a single-file HTML report.

  • It goes against the HTML specification and way of working.

And the solution of separate files is imo not user friendly. User friendlyness > "HTML way of working". Also single-file html reports as output for tools isn't that uncommon imo.

  • We're forcing us to limit to 1 html file in the future. If we would ever want add features like lazy loading parts of the report, we're unable to do so without breaking the azure devops publisher.

Nothing is forced, this is one option. Lazy loading can be a new html report feature instead of replacing the old one. In fact I would prefer it as a new reporter so you still have the option of just generating one self-containing HTML file.

  • It's a generic HTML file publisher.

    • This might sound like a lame excuse, but I like to call things what they are. Why not call it a html-file-publisher instead?

Because that's not discoverable, and because I wanted to contribute something that would spread awareness of mutation-testing-elements.

  • Since the publisher doesn't know anything about the mutation report, it cannot add logic later. Something like inline annotations come to mind: (for example, how eslint does it in github: https://github.com/hallee/eslint-action/blob/master/screenshots/annotation.png), or add a summary of the mutation score somewhere. This won't be possible to add these kind of features if we don't have the data.

We can also support the json report for this.

  • Maintainability.

    • I don't think loading an iframe in an iframe is great for maintainability. What if Microsoft decides to not allow iframes inside their iframe?

I doubt it, but we'll cross that bridge when we come it it. Microsoft could decide to stop with azure devops as well 🤷‍♀️

  • We're dependent on an external dependency (i.e. iframe resizer), so we need to maintain that dependency.

Widely used so not worried about that. We also don't use any fancy features.

  • We're integrating 3 things (azure devops, mutation testing elements and the resizer) into one page, it's just a matter of time before this breaks IMO. Creating e2e tests for it which test the integration will help us with that, but those are also expensive to create and maintain.

My proposal is to create a new publisher that works with the report JSON and rebrand this one as a, more generic, html-file publisher.

@hugo-vrijswijk @richardwerkman @simondel @Mobrockers What are your thoughts?

We could publish a html-file publisher, but I intend for this extension to have specific features related to mutation testing later. As we discussed before I plan to also support JSON reports by loading it in mutation-testing-elements. At that point we could add advanced features like pull request annotation.

@richardwerkman
Copy link
Member

You force other frameworks which want to align on our mutation testing report JSON and HTML report to use a single file report. This is pretty hostile IMO. You might want to load other external resources on your html page, you now force all those resources to be embedded. Another scenario is that they might only want to support our json format, and are not be ready to support our HTML reporter. Too bad, you will not be supported.

By choosing the json publishing you will force other frameworks to have a json reporter. Either way the frameworks will have to fit our reporting standard. I can imagine some frameworks would not like to dirty their reporters by adding a json reporter.

And I would like to remind you that we offer this as a service to other frameworks. But that doesn't mean we have to adapt to all other frameworks. If they want to use our publisher they should adapt to our standards. I think it should not be the other way around. And if they would like to see a change. As Rouke said, it's open source so they can work on it themselves.

And the solution of separate files is imo not user friendly. User friendlyness > "HTML way of working". Also single-file html reports as output for tools isn't that uncommon imo.

I second Roukes opinion here. Singe file outputs are way more user friendly than multi file outputs. Users don't care that the code is ugly (not html way of working) in the report. As long as it works and can be easily used and shared.

@rajbos
Copy link

rajbos commented May 19, 2020

Regarding this:

And the solution of separate files is imo not user friendly. User friendlyness > "HTML way of working". Also single-file html reports as output for tools isn't that uncommon imo.

I recently spent a lot of time working as a 'user' to join multiple Stryker runs into one result view on a .NET projects with lots of solutions. I did so by running Stryker on each solution by itself and then joining the json result files. After that I was done (and happy as well, took quite some time!). I don't like that there would be another step of injecting the json file into the HTML file.

One extra consideration is having that clear separation between 'data' (the json) and the 'UI' (the HTML): I like that a lot: the data can be used anywhere (extra calculations somewhere else?, version tracking?) while the report stays the same.
That also enables scenarios of having the data on your own (external) URL without any issues, which can be beneficiary for corporate settings.

@rouke-broersma
Copy link
Member

rouke-broersma commented May 19, 2020

Regarding this:

And the solution of separate files is imo not user friendly. User friendlyness > "HTML way of working". Also single-file html reports as output for tools isn't that uncommon imo.

I recently spent a lot of time working as a 'user' to join multiple Stryker runs into one result view on a .NET projects with lots of solutions. I did so by running Stryker on each solution by itself and then joining the json result files. After that I was done (and happy as well, took quite some time!). I don't like that there would be another step of injecting the json file into the HTML file.

Let me rephrase. I don't think, in the default case of using a html report generated by stryker, having to work with multiple files is the most user friendly. Of course if you have other requirements you don't want to be forced to use our html reporting solution, which is why the json reporter is also available. Nevertheless, if you select the html reporter in stryker-net, it is my opinion that it is most user friendly to get one single file report that can be shared, saved, moved around without risk of breaking links to other files which makes the html report non-functional.

One extra consideration is having that clear separation between 'data' (the json) and the 'UI' (the HTML): I like that a lot: the data can be used anywhere (extra calculations somewhere else?, version tracking?) while the report stays the same.

As a developer this can be nice to have, which is why we have a json reporter.

That also enables scenarios of having the data on your own (external) URL without any issues, which can be beneficiary for corporate settings.

Json reporter :)

@rajbos
Copy link

rajbos commented May 19, 2020

Wait, let's take a step back: the json reporter creates only the json file, correct?
And the html reporter creates the same json file, with an html file that uses the json to display the data? Just making sure that I have the basics correct :-).

@richardwerkman
Copy link
Member

@rajbos Yes, they both output the same json. But the html reporter adds some html and js to display it.

@rouke-broersma
Copy link
Member

Wait, let's take a step back: the json reporter creates only the json file, correct?
And the html reporter creates the same json file, with an html file that uses the json to display the data? Just making sure that I have the basics correct :-).

In stryker net the json reporter writes a json file to disk, the html reporter writes a self-containing html file to disk. In strykerjs and stryker4s the html reporter writes three files to disk. A js file containing the mutation-testing-elements html elements definition, a json file containing the json report and a html file that loads the Javascript, and json and combines them by placing the mutation-testing-elements report element in the DOM with the json file as source.

That difference is why this extension can only support the stryker net html report currently, until we support uploading the json report and loading that report in a mutation-testing-elements bundled with this extension.

@rajbos
Copy link

rajbos commented May 19, 2020

Thanks for the clarification :-)

@nicojs
Copy link
Member Author

nicojs commented May 20, 2020

Ok, thanks @Mobrockers for your explanation so far. It's clear you feel strongly about this, so feel free to close this issue.

Some responses to your feedback:

And actually, having a need/want to embed their custom html/css/js on top of the report is a perfect use case for a HTML report in the plugin

I see the HTML reporter and azure-devops-mutationreport-publisher as two separate entities. I might want to have my own Stryker logo in my HTML report, but I would expect no logo (or a generic one) in the azure-devops-mutationreport-display tool. (I know we disagree here, no need to point it out).

We can also support the JSON report for this.

You want me to open this issue?

Widely used so not worried about that. We also don't use any fancy features.

Widely used is not a guarantee, especially in the JS world. Believe me, I know 😔. Not using any fancy features is a good start (although loading it via a data url might be considered "fancy", I don't have enough experience to know this). There is also the issue of making sure you're dependencies are up-to-date with regards to security findings. If you need help maintaining the dependencies, let me know.

@rouke-broersma
Copy link
Member

Ok, thanks @Mobrockers for your explanation so far. It's clear you feel strongly about this, so feel free to close this issue.

Some responses to your feedback:

And actually, having a need/want to embed their custom html/css/js on top of the report is a perfect use case for a HTML report in the plugin

I see the HTML reporter and azure-devops-mutationreport-publisher as two separate entities. I might want to have my own Stryker logo in my HTML report, but I would expect no logo (or a generic one) in the azure-devops-mutationreport-display tool. (I know we disagree here, no need to point it out).

Sure not having a logo might be acceptable, but do you also expect that the report behaves (possibly) completely different from what you get native from your tool, because for example a mutation testing framework might support our schema but has a completely different implementation of the html report, instead of mutation-testing-elements?

We can also support the JSON report for this.

You want me to open this issue?

This issue already contains most of the discussion about a json only report so keeping this issue open as a feature request seems fine to me, feel free to open a new issue for it if you think there's too much clutter due to the discussion.

Widely used so not worried about that. We also don't use any fancy features.

Widely used is not a guarantee, especially in the JS world. Believe me, I know 😔. Not using any fancy features is a good start (although loading it via a data url might be considered "fancy", I don't have enough experience to know this). There is also the issue of making sure you're dependencies are up-to-date with regards to security findings. If you need help maintaining the dependencies, let me know.

I don't think the data url is a problem, I also don't think it's required to get this working. I should be able to just use the srcDoc attribute I believe but I had some issues with it and someone suggested data url so I used that instead.

I enabled dependabot but I'm not sure what all the options mean. If there are any options that should be enabled or if there are other dependency management things we should consider please let me know or create a new issue for it.

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

No branches or pull requests

4 participants