Skip to content

Conversation

@mikecirioli
Copy link
Contributor

@mikecirioli mikecirioli commented Feb 22, 2022

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main 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

HTML element attribute id names have changed in newer versions of Jenkins (2.33x.x), causing a few unit tests to break. This PR updates and fixes them.

Comment on lines +71 to +75
<dependency>
<groupId>com.google.errorprone</groupId>
<artifactId>error_prone_annotations</artifactId>
<version>2.10.0</version>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

was excluded for script-security do we need to version manage this or should it be excluded if coming from core via guava?


HtmlPage configFiles = wc.goTo("configfiles");
HtmlAnchor removeAnchor = configFiles.getDocumentElement().getFirstByXPath("//a[contains(@onclick, 'removeConfig?id=" + CONFIG_ID + "')]");
HtmlAnchor removeAnchor = configFiles.getDocumentElement().getFirstByXPath("//a[contains(@data-url, 'removeConfig?id=" + CONFIG_ID + "')]");
Copy link
Member

@jtnord jtnord Feb 23, 2022

Choose a reason for hiding this comment

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

HtmlAnchor removeAnchor = configFiles.getDocumentElement().getFirstByXPath("//a[contains(@data-url, 'removeConfig?id=" + CONFIG_ID + "')|contains(@onclick, 'removeConfig?id=" + CONFIG_ID + "')]");

should (if I got my XPath correct) have allowed to fix a PCT issue without bumping the core to a non LTS making upgrades for LTS users harder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

basically, your saying to make this test backwards compatible with older jenkins?

Copy link
Member

@jtnord jtnord Feb 23, 2022

Choose a reason for hiding this comment

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

I am saying you could have done that to make it backwards compatible with older Jenkins versions. As it is a test only issue and the plugin will work after an upgrade of Jenkins, the current way is not making it any harder for users (there would be no broken plugins post upgrade) so it is fine as is. (hence my approval)

@ampuscas ampuscas mentioned this pull request Apr 29, 2022
6 tasks
@ampuscas
Copy link

@mikecirioli starting from your PR, I have created the PR #199 where I merged master and updated some dependencies because the build was failing due to Maven enforcer rule, so this PR becomes obsolete.

@basil
Copy link
Member

basil commented May 1, 2022

Suggest closing in favor of #200.

@jmMeessen jmMeessen merged commit f862167 into jenkinsci:master May 2, 2022
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.

7 participants