jenkins icon indicating copy to clipboard operation
jenkins copied to clipboard

[JENKINS-69113] renovate progressBar

Open mawinter69 opened this issue 1 year ago • 4 comments

The progressbar used at various places is replaced by a div/span combination and gets a new styling that is in sync with other ui elements. It is a little bit higher, has rounded corners and some padding, the minwidth is set to 10% so we always have at least a bit to show, it also avoids that the border is not rounded, e.g. when we're at just 2% and the progressbar is only 2px wide. Animation is now based on css and no longer a gif and can also be enabled via a flag. The build progress bar shown on run and console views is now updated dynamically including the tooltip. The progress bar used in progressive rendering is doubled in size to make it more prominent that it is still running (See [JENKINS-72138], this doesn't solve the problem but might lower the chance that people reload the mentioned page because they think nothing happens). This also fixes JENKINS-68631

After Build history widget: image

Executor widget: image

Run view: Here the progress bar will change over time image image

Console view with stuck: image

Progressive rendering (agent build history) with bigger progress bar: image

See JENKINS-69113.

Testing done

Manual testing:

  • create job that runs for 1 minute
  • trigger job (no previous builds) -> progress bar is at 100% and animated (on executor widgets, run page and console page)
  • trigger job (with previous builds) -> progress bar is animated and increases over time

To see stuck jobs

  • Create job that does nothing
  • run the job several times
  • Change job to sleep for 1 minute
  • run job -> the progressbar should now be red animated.

Tested for freestyle and pipeline jobs

Proposed changelog entries

  • JENKINS-69113, renovate progressBar

Proposed upgrade guidelines

N/A

### Submitter checklist
- [x] The Jira issue, if it exists, is well-described.
- [x] The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see [examples](https://github.com/jenkins-infra/jenkins.io/blob/master/content/_data/changelogs/weekly.yml)). Fill in the **Proposed upgrade guidelines** section only if there are breaking changes or changes that may require extra steps from users during upgrade.
- [x] There is automated testing or an explanation as to why this change has no tests.
- [x] New public classes, fields, and methods are annotated with `@Restricted` or have `@since TODO` Javadocs, as appropriate.
- [x] New deprecations are annotated with `@Deprecated(since = "TODO")` or `@Deprecated(forRemoval = true, since = "TODO")`, if applicable.
- [x] New or substantially changed JavaScript is not defined inline and does not call `eval` to ease future introduction of Content Security Policy (CSP) directives (see [documentation](https://www.jenkins.io/doc/developer/security/csp/)).
- [x] For dependency updates, there are links to external changelogs and, if possible, full differentials.
- [x] For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

### Maintainer checklist
- [ ] There are at least two (2) approvals for the pull request and no outstanding requests for change.
- [ ] Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
- [ ] Changelog entries in the pull request title and/or **Proposed changelog entries** are accurate, human-readable, and in the imperative mood.
- [ ] Proper changelog labels are set so that the changelog can be generated automatically.
- [ ] If the change needs additional upgrade steps from users, the `upgrade-guide-needed` label is set and there is a **Proposed upgrade guidelines** section in the pull request title (see [example](https://github.com/jenkinsci/jenkins/pull/4387)).
- [ ] If it would make sense to backport the change to LTS, a Jira issue must exist, be a _Bug_ or _Improvement_, and be labeled as `lts-candidate` to be considered (see [query](https://issues.jenkins.io/issues/?filter=12146)).

mawinter69 avatar Jan 02 '24 00:01 mawinter69

Looking good!

I was looking at doing something similar last year, but never finished it. Here’s the branch if it’s at all helpful - https://github.com/jenkinsci/jenkins/compare/master...janfaracik:jenkins:progress-bar-new?expand=1.

https://github.com/jenkinsci/jenkins/assets/43062514/1981bd2d-370d-4ad7-ab6e-af49a016316c

A few things -

  • I think it’d be best to use the existing colour palette for consistency rather than using the colours from the existing progress bar, e.g. using the accent colour for progress, error colour for errors etc.
  • I think there should be separate animations for progress/indeterminate states, e.g. a soft pulse for progress, horizontal translation when indeterminate
  • It’s a little chunky at the moment and draws a lot of attention - maybe that’s okay though?

Let me know your thoughts 👍

janfaracik avatar Jan 03 '24 13:01 janfaracik

Looking good!

I was looking at doing something similar last year, but never finished it. Here’s the branch if it’s at all helpful - master...janfaracik:jenkins:progress-bar-new?expand=1 (compare).

Screen.Recording.2024-01-03.at.13.51.48.mov A few things -

  • I think it’d be best to use the existing colour palette for consistency rather than using the colours from the existing progress bar, e.g. using the accent colour for progress, error colour for errors etc.
  • I think there should be separate animations for progress/indeterminate states, e.g. a soft pulse for progress, horizontal translation when indeterminate
  • It’s a little chunky at the moment and draws a lot of attention - maybe that’s okay though?

Let me know your thoughts 👍

I'm a big fan of your proposed implementation of the progress bars. I agree we should stick with the current color palette for consistency :)

NotMyFault avatar Jan 03 '24 13:01 NotMyFault

The animation is now only used in the widgets when the progress is unknown or is is set via an option. Using existing color palette now moved the css to separate file.

mawinter69 avatar Jan 03 '24 20:01 mawinter69

Should the padding be a bit bigger maybe? like this: image

mawinter69 avatar Jan 03 '24 22:01 mawinter69

Breaks jenkins.branch.NoTriggerBranchPropertyTest#suppressNothing and jenkins.branch.NoTriggerBranchPropertyTest#suppressEvents:

java.lang.AssertionError: 

Expected: is <2>
     but: was <3>
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
	at jenkins.branch.NoTriggerBranchPropertyTest.suppressNothing(NoTriggerBranchPropertyTest.java:135)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:656)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
	at java.base/java.lang.Thread.run(Thread.java:1583)

basil avatar Feb 21 '24 18:02 basil

I wonder how this change could cause this error. This change only affect jelly/js files so it is pure UI

mawinter69 avatar Feb 21 '24 18:02 mawinter69

Are you implying that this PR may not be the cause of the test failures? The tests deterministically fail with this PR and pass when this PR is reverted, so this PR is the (proximate) cause of the test failures.

basil avatar Feb 21 '24 19:02 basil

I also got here via git bisect. NoTriggerBranchPropertyTest runs JenkinsRule.configRoundtrip so it does exercise GUI code.

This PR also causes a failure in the ATH BuildHistoryTest (both test cases) at https://github.com/jenkinsci/acceptance-test-harness/blob/0ac2b663128e2c20bfdcacbf095496bd7c6b43ab/src/main/java/org/jenkinsci/test/acceptance/po/BuildHistory.java#L31.

jglick avatar Feb 23 '24 17:02 jglick

It's definitely happening after configRoundTrip But I think it is not related to this change. When I add a 30 sec sleep after the configRoundTrip, then also the current master of branch-api plugin built against 2.426.1 fails. Isn't there an automatic first run triggered after saving the configuration?

mawinter69 avatar Feb 23 '24 19:02 mawinter69

Thanks, I can take a look at whether NoTriggerBranchPropertyTest is just overly sensitive to timing.

jglick avatar Feb 23 '24 19:02 jglick