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

Support forthcoming PCT multimodule mode #2234

Merged
merged 2 commits into from
Mar 23, 2023
Merged

Conversation

basil
Copy link
Member

@basil basil commented Mar 23, 2023

jenkinsci/plugin-compat-tester#488 discusses the multimodule project branch of PCT and Maven HPI plugin, which runs PCT against entire repositories rather than individual plugin subdirectories. Trying this out in https://github.com/jenkinsci/bom/pull/1879/checks?check_run_id=12209541377 we started running the test-harness/ and integrations/ modules of Configuration as Code under PCT for the first time, turning up four new failures. This PR refreshes this repository; without introducing regression, it also ensures that those four new failures won't occur in the forthcoming multi-module aware PCT. See self-review for an explanation of the changes. To test this, I ran PLUGINS=configuration-as-code TEST=io.jenkins.plugins.casc.GlobalLibrariesTest,io.jenkins.plugins.casc.ConfigurationAsCodeTest,io.jenkins.plugins.casc.SchemaGenerationSanitisationTest,io.jenkins.plugins.casc.impl.configurators bash local-test.sh with the new multi-module aware PCT. Before this PR, I saw the four failures described above. After this PR, the test passes.

@@ -16,27 +16,21 @@
<jackson.version>2.14.2</jackson.version>
<enforcer.skip>true</enforcer.skip> <!-- I give up, too much effort to maintain enforcer here, should probably try clear out all enforcer only deps and see if it works -->
<netty.version>4.1.81.Final</netty.version>
<jenkins.version>2.387.1</jenkins.version>
Copy link
Member Author

Choose a reason for hiding this comment

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

Using jenkins.version from the multi-module project root. Too confusing to have multiple jenkins.version values in different modules: if a newer version is needed, just bump it in the multi-module project root and be done with it.

Copy link
Member

@timja timja Mar 23, 2023

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

<junixsocket.version>2.5.1</junixsocket.version>
<kotlin-stdlib.version>1.7.20</kotlin-stdlib.version>
Copy link
Member Author

Choose a reason for hiding this comment

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

No longer needed to satisfy Enforcer.

Comment on lines -29 to -34
<exclusions>
<exclusion>
<groupId>org.json</groupId>
<artifactId>json</artifactId>
</exclusion>
</exclusions>
Copy link
Member Author

Choose a reason for hiding this comment

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

No longer needed to satisfy Enforcer.

@@ -49,7 +43,6 @@
<dependency>
<groupId>com.coravy.hudson.plugins.github</groupId>
<artifactId>github</artifactId>
<version>1.36.0</version>
Copy link
Member Author

Choose a reason for hiding this comment

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

Now in jenkinsci/bom.

Comment on lines -75 to -79
<dependency>
<groupId>org.apache.ivy</groupId>
<artifactId>ivy</artifactId>
<version>2.5.1</version>
</dependency>
Copy link
Member Author

Choose a reason for hiding this comment

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

No longer needed to satisfy Enforcer.

@@ -17,4 +17,4 @@ unclassified:
strategyId: 2
- gitHubForkDiscovery:
strategyId: 3
trust: "trustPermission"
trust: "gitHubTrustPermissions"
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing this error when both GitHub Branch Source and GitLab Branch Source are on the classpath:

java.lang.AssertionError: 

Expected: an instance of org.jenkinsci.plugins.github_branch_source.ForkPullRequestDiscoveryTrait$TrustPermission
     but: <io.jenkins.plugins.gitlabbranchsource.ForkMergeRequestDiscoveryTrait$TrustPermission@38d7261f> is a io.jenkins.plugins.gitlabbranchsource.ForkMergeRequestDiscoveryTrait$TrustPermission
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
	at io.jenkins.plugins.casc.GlobalLibrariesTest.configure_global_library_using_github(GlobalLibrariesTest.java:49)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:613)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:299)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:293)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.lang.Thread.run(Thread.java:829)

Comment on lines -69 to -73
<dependency>
<groupId>com.google.code.findbugs</groupId>
<artifactId>jsr305</artifactId>
<version>3.0.2</version>
</dependency>
Copy link
Member Author

Choose a reason for hiding this comment

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

Not imported directly and therefore not necessary as a dependency.

@@ -321,7 +321,7 @@ public void string_to_literal_in_export() throws Exception {
@Test
public void testHtmlDocStringRetrieval() throws Exception {
String expectedDocString = "<div>\n"
+ " If checked, this will allow users who are not authenticated to access Jenkins in a read-only mode.\n"
+ " If checked, this will allow users who are not authenticated to access Jenkins\n in a read-only mode.\n"
Copy link
Member Author

Choose a reason for hiding this comment

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

Adapting to jenkinsci/jenkins#6863.

@@ -15,7 +15,7 @@ public class SchemaGenerationSanitisationTest {

@Test
public void testRetrieveDocStringFromAttribute() {
String expectedDocString = "If checked, this will allow users who are not authenticated to access Jenkins in a read-only mode.";
String expectedDocString = "If checked, this will allow users who are not authenticated to access Jenkins\n in a read-only mode.";
Copy link
Member Author

Choose a reason for hiding this comment

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

Adapting to jenkinsci/jenkins#6863.

Comment on lines +25 to +26
// The conditions for this test can be false in PCT runs
assumeThat(j.jenkins.getPlugin("matrix-auth"), nullValue());
Copy link
Member Author

Choose a reason for hiding this comment

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

PCT runs with all plugins in the megawar, including matrix-auth, so skip this test in that context. It still runs in the context of this repository's CI build.

Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

LGTM @basil thanks for the PR.

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.

None yet

3 participants