• Icon: Improvement Improvement
    • Resolution: Fixed
    • Icon: Minor Minor
    • openid-plugin
    • None

      For improved test-ability and upgrades, upgrade the parent pom to the 2.x line. The jenkins.version might also be a candidate for upgrades since it's on a line that's old.

          [JENKINS-36499] Migrate openid to parent pom

          While investigating this conversion, I ran into a lot of issues with how the tests ran. Noteably, there are some tests that I don't think will ever pass. I think these test failures will have to be addressed by the maintainers of the plugin. Also, I think there are some legitimate issues with the plugin since I tried using it on the latest version of Jenkins and ran into 404 errors while trying to use the service.

          Kristin Whetstone added a comment - While investigating this conversion, I ran into a lot of issues with how the tests ran. Noteably, there are some tests that I don't think will ever pass. I think these test failures will have to be addressed by the maintainers of the plugin. Also, I think there are some legitimate issues with the plugin since I tried using it on the latest version of Jenkins and ran into 404 errors while trying to use the service.

          Also, the tests won't pass on their own regardless of the changes I've made with htmlunit. I think this plugin might need some expert eyes to try to fix some of the functionality on the newest versions.

          Kristin Whetstone added a comment - Also, the tests won't pass on their own regardless of the changes I've made with htmlunit. I think this plugin might need some expert eyes to try to fix some of the functionality on the newest versions.

          Ran into this when attempting to run `OpenIdLoginServiceTest`s

          Kristin Whetstone added a comment - Ran into this when attempting to run `OpenIdLoginServiceTest`s

          I've had to disable almost every test while attempting to convert this to use the parent pom. I'm perfectly fine with a simpler changeset that uses an older parent pom if it meant that the tests would also pass. I'm not entirely sure if this is a setup issue on my system, as I have witnessed at least part of the openid service working when configuring it manually in the 1.651.1 version of Jenkins. Some of the exceptions I've seen are a result of the jetty server messing with the trace of openid4java: no crumbIssuer should be configured for these tests while the crumbIssuer can't be set/modified after the server starts. (Somehow, both HudsonTestCase and JenkinsRule both start the server then attempt to change the crumbIssuer while I've only seen explicit errors around the situation while using JenkinsRule).

          This might just be an issue on my local setup.

          Kristin Whetstone added a comment - I've had to disable almost every test while attempting to convert this to use the parent pom. I'm perfectly fine with a simpler changeset that uses an older parent pom if it meant that the tests would also pass. I'm not entirely sure if this is a setup issue on my system, as I have witnessed at least part of the openid service working when configuring it manually in the 1.651.1 version of Jenkins. Some of the exceptions I've seen are a result of the jetty server messing with the trace of openid4java: no crumbIssuer should be configured for these tests while the crumbIssuer can't be set/modified after the server starts. (Somehow, both HudsonTestCase and JenkinsRule both start the server then attempt to change the crumbIssuer while I've only seen explicit errors around the situation while using JenkinsRule). This might just be an issue on my local setup.

          Nope, the problem with the failing test appears when trying to test with the jenkins.ci server from the master branch of the current test suite. The only option in trying to convert all these tests is to:a
          a) ignore all the tests and mark them as something that has to be fixed in the future
          b) accept the changeset with failing tests knowing that the solution has to be found in the future

          Of the 2 options, the 2nd seems more favorable to me, simply from the perspective that the changes go into the codebase.

          The major issue I've seen in the test case conversion is the openid4java throwing errors when the crumbIssuer can't be set after the service has been started. This shows up in old and new tests, and there's really no good way to set up that particular setting. (The jetty server errors on stop/restart and any other overrides aren't a good 1-1 replacement)

          Kristin Whetstone added a comment - Nope, the problem with the failing test appears when trying to test with the jenkins.ci server from the master branch of the current test suite. The only option in trying to convert all these tests is to:a a) ignore all the tests and mark them as something that has to be fixed in the future b) accept the changeset with failing tests knowing that the solution has to be found in the future Of the 2 options, the 2nd seems more favorable to me, simply from the perspective that the changes go into the codebase. The major issue I've seen in the test case conversion is the openid4java throwing errors when the crumbIssuer can't be set after the service has been started. This shows up in old and new tests, and there's really no good way to set up that particular setting. (The jetty server errors on stop/restart and any other overrides aren't a good 1-1 replacement)

          I had to bump the version beyond the 1.580 bump in PR11 to handle JENKINS-25019. The left over failures would be related to this.

          Unfortunately, the problems left over from this pom bump are not related to the issues that I found earlier. I have actually fixed those issues. The new issue is actually related to serving up the openid xrds.jelly; either it can't be found or the proxy service is in error (fakeproxy.jenkins-ci.org: Name or service not known). This might be related to the move from jenkins-ci.org which I think the maintainer of the plugin should deal with. The "not found 404" errors I'm seeing I think relate to the actual location of the xrds.jelly file. JenkinsRule changed a bunch of things around with accessing files, and this is one of the things that was affected by that.

          Kristin Whetstone added a comment - I had to bump the version beyond the 1.580 bump in PR11 to handle JENKINS-25019 . The left over failures would be related to this. Unfortunately, the problems left over from this pom bump are not related to the issues that I found earlier. I have actually fixed those issues. The new issue is actually related to serving up the openid xrds.jelly; either it can't be found or the proxy service is in error (fakeproxy.jenkins-ci.org: Name or service not known). This might be related to the move from jenkins-ci.org which I think the maintainer of the plugin should deal with. The "not found 404" errors I'm seeing I think relate to the actual location of the xrds.jelly file. JenkinsRule changed a bunch of things around with accessing files, and this is one of the things that was affected by that.

          To get around this when using the new test harness, the version must minimally be 1.586 since the change wasn't backported to the 1.580 line.

          Kristin Whetstone added a comment - To get around this when using the new test harness, the version must minimally be 1.586 since the change wasn't backported to the 1.580 line.

          All tests are passing. I'd appreciate some reviews and eventually a merge from the plugin maintainer. There are still changes that the openid plugin maintainer will want to review. PR10

          Kristin Whetstone added a comment - All tests are passing. I'd appreciate some reviews and eventually a merge from the plugin maintainer. There are still changes that the openid plugin maintainer will want to review. PR10

          Code changed in jenkins
          User: Stephen Connolly
          Path:
          pom.xml
          src/main/java/hudson/plugins/openid/OpenIdExtension.java
          src/main/java/hudson/plugins/openid/OpenIdSession.java
          src/main/java/hudson/plugins/openid/OpenIdSsoSecurityRealm.java
          src/main/java/hudson/plugins/openid/OpenIdUserProperty.java
          src/main/java/hudson/plugins/openid/StaticResourceServer.java
          src/main/resources/hudson/plugins/openid/OpenIdLoginService/_openid-form-body.jelly
          src/main/resources/hudson/plugins/openid/OpenIdLoginService/loginFragment.jelly
          src/main/resources/hudson/plugins/openid/OpenIdLoginService/onAssociationSuccess.jelly
          src/main/resources/hudson/plugins/openid/OpenIdUserProperty/config.jelly
          src/main/resources/index.jelly
          src/test/java/hudson/plugins/openid/OpenIdAXEmailAttributesTest.java
          src/test/java/hudson/plugins/openid/OpenIdLoginServiceTest.java
          src/test/java/hudson/plugins/openid/OpenIdSsoSecurityRealmTest.java
          src/test/java/hudson/plugins/openid/OpenIdTestCase.java
          src/test/java/hudson/plugins/openid/OpenIdTestService.java
          http://jenkins-ci.org/commit/openid-plugin/bc0db50d1e67af115230f1e5c960a064e72effa7
          Log:
          Merge pull request #10 from kwhetstone/master

          JENKINS-36499 Update to the new Parent POM

          Compare: https://github.com/jenkinsci/openid-plugin/compare/37ec8d0fed54...bc0db50d1e67

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Stephen Connolly Path: pom.xml src/main/java/hudson/plugins/openid/OpenIdExtension.java src/main/java/hudson/plugins/openid/OpenIdSession.java src/main/java/hudson/plugins/openid/OpenIdSsoSecurityRealm.java src/main/java/hudson/plugins/openid/OpenIdUserProperty.java src/main/java/hudson/plugins/openid/StaticResourceServer.java src/main/resources/hudson/plugins/openid/OpenIdLoginService/_openid-form-body.jelly src/main/resources/hudson/plugins/openid/OpenIdLoginService/loginFragment.jelly src/main/resources/hudson/plugins/openid/OpenIdLoginService/onAssociationSuccess.jelly src/main/resources/hudson/plugins/openid/OpenIdUserProperty/config.jelly src/main/resources/index.jelly src/test/java/hudson/plugins/openid/OpenIdAXEmailAttributesTest.java src/test/java/hudson/plugins/openid/OpenIdLoginServiceTest.java src/test/java/hudson/plugins/openid/OpenIdSsoSecurityRealmTest.java src/test/java/hudson/plugins/openid/OpenIdTestCase.java src/test/java/hudson/plugins/openid/OpenIdTestService.java http://jenkins-ci.org/commit/openid-plugin/bc0db50d1e67af115230f1e5c960a064e72effa7 Log: Merge pull request #10 from kwhetstone/master JENKINS-36499 Update to the new Parent POM Compare: https://github.com/jenkinsci/openid-plugin/compare/37ec8d0fed54...bc0db50d1e67

          This was delivered and released in openid 2.2

          Kristin Whetstone added a comment - This was delivered and released in openid 2.2

            kwhetstone Kristin Whetstone
            kwhetstone Kristin Whetstone
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated:
              Resolved: