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 junit 5; Migrate non-forked junit 3 test cases #593

Merged
merged 11 commits into from
Nov 29, 2022
Merged

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Oct 13, 2022

Migrate existing junit 3 tests to junit 5 with standard ParameterizedTest facility

This uses @MethodSource providing the various ChannelRunner instances used for testing.

Migrated existing assert/assume calls impacted tests to the new jupiter api.

Overall this covers more combinations than the previous framework as some of them were actually not covered previously for some reason, even though they were extending RmiTestBase.

Keeping existing junit 3 test cases from forked packages as the proper approach here would rather be to pull new versions.

  • 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

Migrate existing junit 3 tests to junit 5 with standard ParameterizedTest facility

This uses @MethodSource providing the various ChannelRunner instances
used for testing.

Migrated existing assert/assume calls impacted tests to the new jupiter api.

Overall this covers more combinations than the previous framework as
some of them were actually not covered previously for some reason.

Keeping existing junit 3 test cases from forked packages as the proper
approach here would rather be to pull new versions.
@Vlatombe Vlatombe requested a review from jglick October 13, 2022 15:07
@Vlatombe Vlatombe added the test label Oct 13, 2022
@jglick
Copy link
Member

jglick commented Oct 13, 2022

@Vlatombe
Copy link
Member Author

#593 (checks) flake or real?

~50% flake locally if I recall properly.

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.

OK I think. JUnit 5 is pretty opaque to me.

Does this patch make Surefire timeouts apply automatically?

src/test/java/hudson/remoting/ChannelRunners.java Outdated Show resolved Hide resolved

final ExecutionException e = assertThrows(ExecutionException.class, f::get);
assertThat(e.getCause(), instanceOf(IOException.class));
final ExecutionException e = assertThrows(ExecutionException.class, () -> f.get(2, TimeUnit.SECONDS));
Copy link
Member

Choose a reason for hiding this comment

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

Correcting flake I guess?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

@jglick
Copy link
Member

jglick commented Oct 14, 2022

Seems ChannelTest can still hang?

@jglick
Copy link
Member

jglick commented Oct 24, 2022

Do you plan to look into #593 (comment), or shall I just merge this as is?

@Vlatombe
Copy link
Member Author

Vlatombe commented Nov 4, 2022

Added a timeout, I don't plan to investigate more time into the hanging test.

@jglick
Copy link
Member

jglick commented Nov 4, 2022

ca83285 did not work it seems:

00:06:57.224  [INFO] Running hudson.remoting.ChannelTest
00:17:11.572  Sending interrupt signal to process

@jglick
Copy link
Member

jglick commented Nov 29, 2022

Maybe it worked so far as it went, but each test method gets run up to five times, and there are up to six parameter values, so spending a minute on each run in the worst case can be a lot.

Hmm, but it seems that by default timeouts are not preëmptive—if you exceed the timeout (but the method completes), the test fails, but that is no good for ensuring CI completes in time.

@jglick
Copy link
Member

jglick commented Nov 29, 2022

Some really weird stuff like

@Rule
public RuleChain chain = RuleChain.outerRule(selector)
.around(new RepeatRule())
.around(new Timeout(10, TimeUnit.SECONDS));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants