-
Notifications
You must be signed in to change notification settings - Fork 155
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
Better HTML report model #254
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I added some comments.
// Initialize sample config | ||
benchmarkResult.scenarios.forEach((scenario, scenarioIndex, scenarios) => { | ||
scenario.samples.forEach((sample, sampleIndex, samples) => { | ||
sample.color = `hsl(${scenarioIndex * 360 / scenarios.length}, ${100 - 80 * sampleIndex / samples.length}%, ${30 + 40 * sampleIndex / samples.length}%)`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how important it is to distinguish the different shades of green (=different build operations for the same experiment), it seems more important to distinguish the green from the red (same build operations for different experiments).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said I'm open to suggestions.
let maxMeasuredIterations = Math.max(...(benchmarkResult.scenarios.map(scenario => iterations(scenario, Phase.MEASURE).length))); | ||
let chart = new Chart(ctx, { | ||
type: 'line', | ||
var app = new Vue({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var app = new Vue({ | |
const app = new Vue({ |
No need to leave this mutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed this and others.
@@ -148,12 +142,21 @@ | |||
</div> | |||
</div> | |||
<footer> | |||
<span>Gradle Profiler version {{ environment.profilerVersion }}</span> | |||
<span>Gradle Profiler version {{ benchmarkResult.environment.profilerVersion }}</span> | |||
</footer> | |||
</div> | |||
<script> | |||
Array.prototype.unique = function() { return [...new Set(this)]; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unique
function is now unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it and moved the utility functions into Array
prototype functions.
} | ||
} | ||
}; | ||
|
||
function convertScenarioToChart(scenario, sample, sorted, color) { | ||
var data = iterations(scenario, Phase.MEASURE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var data = iterations(scenario, Phase.MEASURE) | |
const data = iterations(scenario, Phase.MEASURE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I still wonder if we can do something about the colors. Let's merge this PR and discuss to see if we can come up with something.
Vue.js allows us to better model things. Let's make use of it.
test-report.html.zip