Skip to content

Convert trend to ECharts #134

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

Merged
merged 10 commits into from
Jul 30, 2020
Merged

Convert trend to ECharts #134

merged 10 commits into from
Jul 30, 2020

Conversation

timja
Copy link
Member

@timja timja commented Jun 24, 2020

@fqueiruga
Copy link

@uhafner Does the echarts library brings in bootstrap? I'm a bit concerned to put a full bootstrap page on the main build page.

Other than that, it looks quite good.

@timja
Copy link
Member Author

timja commented Jun 24, 2020

No it doesn't:

image

@fqueiruga
Copy link

That's great then!

@uhafner
Copy link
Member

uhafner commented Jun 24, 2020

No, I does not use bootstrap. (It requires jQuery3 plugin up to now).

import java.util.Iterator;
import java.util.NoSuchElementException;

public class TestResultActionIterable implements Iterable {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public class TestResultActionIterable implements Iterable {
public class TestResultActionIterable implements Iterable<BuildResult<AbstractTestResultAction>> {

?

@@ -289,7 +292,10 @@ public TestResult findCorrespondingResult(String id) {

/**
* Generates a PNG image for the test result trend.
*
* Deprecated: Replaced by echarts in TODO
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Deprecated: Replaced by echarts in TODO
* @deprecated Replaced by echarts

and similarly elsewhere.

@jglick
Copy link
Member

jglick commented Jun 24, 2020

it shows passing tests instead of total tests

If I understand the “after” chart it seems like there may be some color distinction between red and purple (~ red + blue)? If so, this is really hard for me to grasp, and I cannot imagine how you would overlay “skipped” on that unless you switched from grey to yellow, at which point you are in a three-dimensional color space… Suggest returning to the original format, which I at least find easier to interpret.

@timja
Copy link
Member Author

timja commented Jun 28, 2020

Thoughts now?

image

@timja timja marked this pull request as ready for review June 28, 2020 15:53
@timja timja requested a review from jglick June 28, 2020 15:53
@skundrik
Copy link

If you want to go fancy you can add a filter by legend. Click to toggle on graph.

@uhafner
Copy link
Member

uhafner commented Jun 28, 2020

If you want to go fancy you can add a filter by legend. Click to toggle on graph.

This is automatically provided by echarts legend.

@timja
Copy link
Member Author

timja commented Jun 28, 2020

If you want to go fancy you can add a filter by legend. Click to toggle on graph.

This is automatically provided by echarts legend.

Didn't notice that before, that's nice!

@timja
Copy link
Member Author

timja commented Jul 8, 2020

@jglick / @olivergondza / @dwnusbaum

Copy link
Member

@olivergondza olivergondza left a comment

Choose a reason for hiding this comment

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

I like this visually. How will this play together with other plugins visualizing test results, like matrix-project?

@timja
Copy link
Member Author

timja commented Jul 8, 2020

I like this visually. How will this play together with other plugins visualizing test results, like matrix-project?

does it call the junit APIs?

If so deprecated those APIs to be replaced with the echarts ones

@olivergondza
Copy link
Member

To the best of my understanding, some at least are referring to generated image: https://github.com/search?q=org%3Ajenkinsci+trendMap&type=Code

Keeping the endpoint in place keeps them from breaking, but the maintainers might not realize this is actually deprecated as they are not calling the Java API. My question was really to see if there is an effort to update all the plugins to avoid breakage and unify the appearance.

@timja
Copy link
Member Author

timja commented Jul 8, 2020

To the best of my understanding, some at least are referring to generated image: github.com/search?q=org%3Ajenkinsci+trendMap&type=Code

Keeping the endpoint in place keeps them from breaking, but the maintainers might not realize this is actually deprecated as they are not calling the Java API. My question was really to see if there is an effort to update all the plugins to avoid breakage and unify the appearance.

To a degree yes, but focusing on the most used ones right now...

If I find time I'll try update one other as an example

@uhafner
Copy link
Member

uhafner commented Jul 8, 2020

Matrix project (and Maven project) do not use a separate UI they just use the charts from the JUnit plugin automatically.
And the trendMap methods are provided by each plugin. I think no other plugin is using these methods of the JUnit plugin. They just use the same copy of the code that produces the maps. These methods should be private within a plugin (they can't since Stapler is not very smart with mapping methods).

@olivergondza
Copy link
Member

Cool, thanks!

@timja
Copy link
Member Author

timja commented Jul 11, 2020

Matrix project seems to work fine:

image

@timja
Copy link
Member Author

timja commented Jul 11, 2020

All seems fine unless someone can point me to something in particular as I've never used those plugins

@timja
Copy link
Member Author

timja commented Jul 23, 2020

@jglick @oleg-nenashev @olivergondza do you need anything else from me?

Thanks 🙏

@olivergondza
Copy link
Member

I am fine with the changes the way they are. Unfortunately, there might be no active maintainer to integrate this so you might very well go ahead and integrate this yourself as soon as there is a consensus.

@timja
Copy link
Member Author

timja commented Jul 23, 2020

I am fine with the changes the way they are. Unfortunately, there might be no active maintainer to integrate this so you might very well go ahead and integrate this yourself as soon as there is a consensus.

Core team doesn't appear to have access, but happy to request permissions, if there's no active maintainer here, (not looking to do much other than fix any fallout if there's any and small patches possibly).

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.

7 participants