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

Upgrade trends and detail views to Bootstrap 5 #272

Merged
merged 20 commits into from
May 2, 2022

Conversation

uhafner
Copy link
Member

@uhafner uhafner commented May 21, 2021

Bootstrap 5 has been released and requires some changes in markup and JS. Additionally, (due to the migration of ECharts to Bootstrap 5) trend charts are now configurable using a dialog.

Upstream PR: jenkinsci/echarts-api-plugin#124

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Additionally, trend charts are now configurable due to an upgrade to the echarts plugin.
It is not required to retain backward compatibility for restricted classes.
Now the general trend options are changed for *all* charts.
So individual trend chart dialogs change the global configuration and the local one.
Now configuration of trend charts in job page and trend charts in details pages use a different dialog.
@uhafner
Copy link
Member Author

uhafner commented Jun 9, 2021

Seems that HTMLUnit does not understand modern JS :-( Browser and IntelliJ do not complain. This is one of the reasons why I removed HTMLUnit tests from my plugins. They break very often due to incompatibilities (and are soooo slow).

EChartsJenkinsApi.prototype.readConfiguration = function (id) {
    const specific = echartsJenkinsApi.readFromLocalStorage(id);
    const common = echartsJenkinsApi.readFromLocalStorage(trendDefaultStorageId);

    return {
        ...specific,
        ...common
    };
}

@timja Any idea? I don't want to modify my code just to make sure that HTMLunit is happy. Why do you use HTMLUnit anyway? Shouldn't UI errors better be caught by ATH tests?

See HtmlUnit/htmlunit#111

@timja
Copy link
Member

timja commented Aug 5, 2021

Everything in this plugin is inherited, probably from when it was split from core.

I’ll try take a peek at this soon and see if I can find a way forward

Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

I thought there was a trend toward getting rid of shared js libs not adding more (using webpack and creating isolated bundles for each plugin).

If this is CSS are we going to have collisions where BS 4 and BS 5 are in the same page
@EstherAF

@timja
Copy link
Member

timja commented Aug 5, 2021

I think only the history page uses bootstrap and that will be explicitly loading bootstrap 4

@timja
Copy link
Member

timja commented Aug 8, 2021

Now failing for HtmlUnit/htmlunit#232

@uhafner
Copy link
Member Author

uhafner commented Aug 9, 2021

I have a testing course in the winter semester again. If it is helpful we could include the JUnit plugin as project and restructure it so that the ATH tests will run inside the plugin build as well. There is already a test at https://github.com/jenkinsci/acceptance-test-harness/blob/master/src/test/java/plugins/JUnitPluginTest.java that just needs to be moved (and maybe extended).

As already mentioned above, I also think using HTMLUnit In general is not very helpful as it is just a (slow and buggy) simulation of a real browser. It works quite well with the static HTML pages of the old style Jenkins UI but not with modern pages that use a lot of other JS libraries. I think it make more sense if we would use unit and integration tests only to verify the Java models and not the browser rendering. Browser render should only be tested (if at all) using ATH.

@timja
Copy link
Member

timja commented Aug 9, 2021

I have a testing course in the winter semester again. If it is helpful we could include the JUnit plugin as project and restructure it so that the ATH tests will run inside the plugin build as well. There is already a test at https://github.com/jenkinsci/acceptance-test-harness/blob/master/src/test/java/plugins/JUnitPluginTest.java that just needs to be moved (and maybe extended).

If you could do that it would be great!

Absolutely onboard with the conversion of these to ATH tests.

(In the meantime if rhino adds support for what we need we'll be able to ship this)

pom.xml Outdated Show resolved Hide resolved
# Conflicts:
#	pom.xml
#	src/main/java/hudson/tasks/junit/History.java
#	src/main/java/hudson/tasks/test/TestResultDurationChart.java
#	src/main/resources/hudson/tasks/junit/History/index.jelly
#	src/main/webapp/history/history.js
This makes it necessary to simplify the trend chart test:
the chart requires JS to render so we check for the container only.
@uhafner uhafner marked this pull request as ready for review May 1, 2022 22:32
@uhafner uhafner requested a review from timja May 1, 2022 22:32
@uhafner uhafner assigned basil and unassigned basil May 1, 2022
@uhafner uhafner requested a review from basil May 1, 2022 22:32
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

good enough with the limitations we have, I'm not sure if the tests are actually testing much now given the endpoints aren't being called with JS disabled but let's get this in so we can move forward in bom

@timja timja merged commit 433a2d8 into jenkinsci:master May 2, 2022
@basil
Copy link
Member

basil commented May 2, 2022

Thank you both. I merged in these changes to jenkinsci/bom#571. Unfortunately, while this solves the problem in bom-weekly, it doesn't solve it for older BOM lines, where this fix isn't present. And due to the way the BOM is structured, any plugin that is used by one BOM line has to be present on all the others. So could this fix please be backported to 1.53[.1] and 1.54[.1]? Here are the instructions for doing so: https://gist.github.com/jglick/86a30894446ed38f918050c1180483e2 Once this is backported to all BOM lines, we can proceed with ripping out bootstrap4-api and popper-api from the BOM in favor of bootstrap5-api and popper2-api. And with those two plugins presumably having well-maintained builds, we can then proceed with JENKINS-45047.

@timja
Copy link
Member

timja commented May 5, 2022

I've pushed origin branches:

https://github.com/jenkinsci/junit-plugin/tree/1.53.x
https://github.com/jenkinsci/junit-plugin/tree/1.54.x

I spent sometime trying to backport it but found it difficult, lots of pom issues.

Feel free to submit PRs to those branches, otherwise probably won't do? I believe you have found a workaround in bom

@basil
Copy link
Member

basil commented May 6, 2022

I somehow managed to find a way to work around this for now, but …

otherwise probably won't do

… in general, if you aren't willing to do backports, then I suggest not increasing the baseline so aggressively.

basil pushed a commit to basil/junit-plugin that referenced this pull request Jul 24, 2022
@basil basil mentioned this pull request Jul 24, 2022
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.

4 participants