Skip to content

Fixed test result graph rendering and added three new dashboard columns#26

Open
japod wants to merge 2 commits intojenkinsci:masterfrom
japod:fixed-test-result-graph-rendering
Open

Fixed test result graph rendering and added three new dashboard columns#26
japod wants to merge 2 commits intojenkinsci:masterfrom
japod:fixed-test-result-graph-rendering

Conversation

@japod
Copy link

@japod japod commented Jun 5, 2019

This PR addresses issues caused by various NPEs coming from trend graph rendering logic that i experienced while using TAP plugin in our local deployment for NPM package testing.

The PR also introduces several dashboard view columns that help with propagating test counter related information to the view and also help with determining compliance of selected jobs with given "golden" scenario based on test counter values.

Attaching several screenshots to illustrate the functionality added:

Config page of the view named "test":
config

Golden, base line, project in a view:
golden-baseline

Test project "initial", incompatible, state:
tap-test-project-1

Corresponding dashboard view:
tap-dashboard1

Test project stabilized:
tap-test-project-stabilized
tap-dashboard2

Test project, clean run:
tap-test-project-clean-run
tap-dashboard3

Test project back to compatible state:
tap-test-project-back-to-compatible
tap-dashboard4

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Looking good, I just need to confirm how to add the dashboard dependency optionally, or if that's not possible if that wouldn't cause issues (with security, updating Jenkins, etc.). In Active Choices Scriptler is an optional dependency, but I think I had to protect against nulls at least when the plug-in was not installed...

Thanks for the PR, sorry for the long delay.

<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>dashboard-view</artifactId>
<version>${dashboard.plugin.version}</version>
Copy link
Member

Choose a reason for hiding this comment

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

The protection against NPEs looks easy to be reviewed and merged. This dependency and the classes added are a bit more complicated to review, I think. Would be easier if this could be an optional dependency, but I am not sure if that wouldn't cause some runtime errors for users without he plug-in (I think in active choices I have an optional dependency, will check what steps were taken for that.

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.

2 participants