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

Disable failing test in UDP broadcast #4467

Merged
merged 1 commit into from
Feb 3, 2020

Conversation

Wadeck
Copy link
Contributor

@Wadeck Wadeck commented Feb 2, 2020

Currently all builds are failing due to failures in UDP broadcast tests. We have a pending PR to remove the feature completely due to security vulnerabilities but before the PR is merged, we need to keep the other PRs passing tests.

Reference for the other one: #4460

FYI @jeffret-b

Copy link
Member

@batmat batmat left a comment

Choose a reason for hiding this comment

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

All for the disabling, but I think we should use @Ignore so the ignored tests still show up in the reports as such.

Thanks!

@@ -55,7 +55,8 @@ private static void updatePort(int newValue) throws Exception {
* Old unicast based clients should still be able to receive some reply,
* as we haven't changed the port.
*/
@Test public void legacy() throws Exception {
Copy link
Member

@batmat batmat Feb 2, 2020

Choose a reason for hiding this comment

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

I think we should use the standard @Ignore annotation, is there a compelling reason not to use this?

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Feb 2, 2020

All for the disabling, but I think we should use @Ignore so the ignored tests still show up in the reports as such.

I thought that the reason to disable was:

We have a pending PR to remove the feature completely due to security vulnerabilities but before the PR is merged, we need to keep the other PRs passing tests.

Since the feature is being removed, is the better choice to delete the tests rather than disable it?

@markjacksonfishing
Copy link

This is a non-binding comment but I agree that if #4460 is in flight disabling is more the way to go

Copy link
Contributor

@res0nance res0nance left a comment

Choose a reason for hiding this comment

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

Fully agree since the removal is in flight we should just disable this test instead of spending cycles on it

@Wadeck Wadeck requested a review from batmat February 3, 2020 08:48
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

Whatever works.

@res0nance res0nance added internal skip-changelog Should not be shown in the changelog labels Feb 3, 2020
@jeffret-b
Copy link
Contributor

I've got lots of approvals on #4460 and only one area of possible concern but haven't heard back from anyone that actually uses it. I'm hopeful we'll get it merged. I've always had failures with these tests on my machine, so I was reluctant to try and fix anything here. I'm in favor of the easiest way to disable the tests until we can get the other PR in.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Prefer to either @Ignore or delete, but good enough for now.

@jglick
Copy link
Member

jglick commented Feb 3, 2020

FTR

java.lang.IllegalAccessException: Can not set static final int field hudson.UDPBroadcastThread.PORT to (int)33848
	at java.base/jdk.internal.reflect.UnsafeFieldAccessorImpl.throwFinalFieldIllegalAccessException(UnsafeFieldAccessorImpl.java:76)
	at java.base/jdk.internal.reflect.UnsafeFieldAccessorImpl.throwFinalFieldIllegalAccessException(UnsafeFieldAccessorImpl.java:100)
	at java.base/jdk.internal.reflect.UnsafeQualifiedStaticIntegerFieldAccessorImpl.setInt(UnsafeQualifiedStaticIntegerFieldAccessorImpl.java:129)
	at java.base/java.lang.reflect.Field.setInt(Field.java:960)
	at hudson.UDPBroadcastThreadTest.updatePort(UDPBroadcastThreadTest.java:51)
	at hudson.UDPBroadcastThreadTest.forceActivateUDPMulticast(UDPBroadcastThreadTest.java:41)

not tied obviously to any SCM commit.

@jglick jglick merged commit ce63fb1 into jenkinsci:master Feb 3, 2020
@res0nance res0nance mentioned this pull request Feb 4, 2020
jsoref pushed a commit to jsoref/jenkins that referenced this pull request Feb 6, 2020
Disable failing test in UDP broadcast
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal skip-changelog Should not be shown in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants