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

Remove LOC displays from coverage graph #1576

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

josephsnyder
Copy link
Member

@josephsnyder josephsnyder commented Jul 14, 2023

To eliminate an error where the Lines Of Code data attempted to access a non-existant yaxis object, remove the plots for LOC from the coverage over time display.

With an update to the design, they may be reintroduced by switching to yaxes in the plot's options object and then reintroducing the data.

Previous State:

image

Current State:
image

Copy link
Collaborator

@williamjallen williamjallen left a comment

Choose a reason for hiding this comment

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

Is there an easy way to re-add the loc(un)tested data to the plot. The code itself looks fine, but I think having the loc data on the plot is valuable information to include.

@josephsnyder
Copy link
Member Author

josephsnyder commented Jul 14, 2023

@williamjallen, @zackgalbreath thought it would be cleaner to have only the one plot and one axis on the page. I was thinking a hover/tooltip like (http://www.flotcharts.org/flot/examples/interacting/) would give us the information we really want without being too confusing.

ETA: Demonstration of Tooltip:
image

Is that reasonable?

@williamjallen
Copy link
Collaborator

@josephsnyder I like that idea. That way, if the coverage drops (or spikes) suddenly you can do a little bit of preliminary exploration to determine the cause.

@josephsnyder josephsnyder force-pushed the fix_coverage_timeline_display branch 2 times, most recently from 5f5a288 to 16d6837 Compare July 17, 2023 14:28
Copy link
Collaborator

@williamjallen williamjallen left a comment

Choose a reason for hiding this comment

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

Actually, one request: Can you add labels to the axes? It's not clear what the x-axis is. I think it also might be good to fix the bounds so it always shows a scale from 0 to 100% on the y-axis. As it is now, it looks like Cmake had a drastic drop, when it really only dropped less than 1%.

Here's a screenshot from CMake:
image

@josephsnyder
Copy link
Member Author

josephsnyder commented Jul 17, 2023

@williamjallen, I don't see a way to pin the axis values to a set number. By adding a step to the x axis label, I think we can clarify that it's supposed to be a build time:

image

@williamjallen
Copy link
Collaborator

@josephsnyder That seems reasonable to me. Perhaps we should also include the date with those timestamps though...

To eliminate an error where the Lines Of Code data attempted to access a
non-existant yaxis object, remove the plots for LOC from the coverage over time
display.

Use the information in tooltip instead of additional plots on the single
graph.
@josephsnyder josephsnyder force-pushed the fix_coverage_timeline_display branch from 16d6837 to de561d5 Compare July 17, 2023 16:45
@williamjallen williamjallen enabled auto-merge July 17, 2023 17:15
@williamjallen williamjallen added this pull request to the merge queue Jul 17, 2023
Merged via the queue into master with commit 999e42e Jul 17, 2023
@williamjallen williamjallen deleted the fix_coverage_timeline_display branch July 17, 2023 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants