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(metrics): add aggregateResultsByModule function #1225

Merged
merged 5 commits into from
Jul 31, 2021

Conversation

nicojs
Copy link
Member

@nicojs nicojs commented Jul 2, 2021

aggregateResultsByModule [(Record<string, MutationTestResult>) => MutationTestResult]

Aggregates multiple reports together into a single report, grouped by module.

Input: resultsByModule The MutationTestResult objects by module name.

Closes #1180

Add `aggregateResultsByModule`, which can be used merge multiple reports into 1.

Closes #1180
@nicojs nicojs changed the title feat/aggregate feat(metrics): add aggregateResultsByModule function Jul 2, 2021
Copy link
Member

@hugo-vrijswijk hugo-vrijswijk left a comment

Choose a reason for hiding this comment

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

Some small comments about the code. But we've also talked about adding another possible field in the schema to allow multiple full reports aggregated inside a single schema (with different thresholds, configs, etc). How does that relate to this? I wouldn't want to merge this and expose it as a public API if we're going to make a very similar functionality soon as well

packages/metrics/src/aggregate.ts Show resolved Hide resolved
config: {},
projectRoot: undefined,
};
expect(aggregateResultsByModule(Object.create(null))).deep.eq(expectedReport);
Copy link
Member

Choose a reason for hiding this comment

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

What if an empty object is passed to the function? This Object.create thing makes me worry it gives unexpected results

Copy link
Member Author

Choose a reason for hiding this comment

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

Object.create(null) is the emptiest object there is. Even more empty than {}, since it won't even have Object.prototype as its prototype

Copy link
Member

Choose a reason for hiding this comment

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

Does {} give a different result?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Then that should probably also have a test, right? {} seems a lot more common 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Added the test

@nicojs
Copy link
Member Author

nicojs commented Jul 3, 2021

But we've also talked about adding another possible field in the schema to allow multiple full reports aggregated inside a single schema (with different thresholds, configs, etc).

I still want that, but I don't want to spend the time for that a.t.m. I actually just want the merging of tests files in the dashboard, and wanted to move that out of the dashboard because users are asking for it as well.

How does that relate to this? I wouldn't want to merge this and expose it as a public API if we're going to make a very similar functionality soon as well

When that functionality lands, I would expect the internals of this function to change so it provides the new format.

@hugo-vrijswijk
Copy link
Member

@nicojs What is the current status of this PR?

@nicojs
Copy link
Member Author

nicojs commented Jul 31, 2021

I forgot about it 😶

@hugo-vrijswijk
Copy link
Member

Ship it!

@hugo-vrijswijk hugo-vrijswijk merged commit bb690b8 into master Jul 31, 2021
@hugo-vrijswijk hugo-vrijswijk deleted the feat/aggregate branch July 31, 2021 18:01
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.

Add support for merging multiple reports together in 1 file
2 participants