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(report): support upload of full report to dashboard #1783

Merged
merged 20 commits into from
Nov 17, 2019

Conversation

nicojs
Copy link
Member

@nicojs nicojs commented Oct 17, 2019

Support having the full HTML report send to the stryker dashboard (https://dashboard.stryker-mutator.io).

See design: stryker-mutator/stryker-handbook#27 and https://github.com/stryker-mutator/stryker-handbook/blob/master/dashboard.md

Changes:

  • Add dashboard options: baseUrl, project, version, module reportType ('mutationScore'/'full').
  • Updated CI providers (travis and circle ci are supported)
  • Update StrykerCli: made it testable and written tests for it.

Fixes #1791

@nicojs nicojs requested a review from simondel October 26, 2019 20:47
@nicojs
Copy link
Member Author

nicojs commented Oct 26, 2019

@simondel I think this is now ready for review. Would you mind taking a look?

@nicojs nicojs changed the title feat(report): support experimental upload of full report to dashboard feat(report): support upload of full report to dashboard Oct 28, 2019
@nicojs
Copy link
Member Author

nicojs commented Nov 7, 2019

@dependabot rebase

@nicojs
Copy link
Member Author

nicojs commented Nov 7, 2019

Well.. head to try 😅

Copy link
Member

@simondel simondel left a comment

Choose a reason for hiding this comment

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

@nicojs Could you look at my comments?

@@ -106,6 +106,7 @@ You can *ignore* files by adding an exclamation mark (`!`) at the start of an ex
* [timeoutFactor](#timeoutFactor)
* [timeoutMS](#timeoutMS)
* [transpilers](#transpilers)
* [dashboard.*](#dashboard)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add this in the correct order?

Copy link
Member

Choose a reason for hiding this comment

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

(Same goes for the description)

import { Config } from '@stryker-mutator/api/config';
import { Logger } from '@stryker-mutator/api/logging';

function deepOption<T extends string, R>(object: { [K in T]?: R }, key: T, valueInterpreter = (value: R) => value) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add comments to explain what this does? Also, I can't find a reference as to a custom valueInterpreter argument

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

import { Config } from '@stryker-mutator/api/config';
import { Logger } from '@stryker-mutator/api/logging';

function deepOption<T extends string, R>(object: { [K in T]?: R }, key: T, valueInterpreter = (value: R) => value) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this function not part of your class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it's not using this. Moreover, I especially don't want to make a mistake and use this, since it will be redirected to be something else (the Commander instance or something)

private toReport(result: mutationTestReportSchema.MutationTestResult): Report {
if (this.options.dashboard.reportType === ReportType.Full) {
return {
result
Copy link
Member

Choose a reason for hiding this comment

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

Don't you have to set the mutationScore if you also set the result?

packages/api/src/config/Config.ts Show resolved Hide resolved
@bartekleon
Copy link
Member

npm run lint:fix should solve most of the issues :/

@nicojs
Copy link
Member Author

nicojs commented Nov 15, 2019

Yeah, you're right! I don't know why I was thinking it was something else. Thanks for the help 😅

@simondel I've resolved/remarked on all issues. Please take a look again. Also implemented the latest design according to stryker-mutator/stryker-handbook#27

@nicojs
Copy link
Member Author

nicojs commented Nov 15, 2019

BTW:

I've set the default report type to "mutationScore", for backward compatibility reasons. Should we log a deprecation warning to specify that it will change in the future? Or wait for the next major release to do that (~1 jan)

@nicojs nicojs merged commit fbb8102 into master Nov 17, 2019
@nicojs nicojs deleted the feat/full-report-to-dashboard branch November 17, 2019 15:59
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.

Full report on the stryker-dashboard
3 participants