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

SOLR-17618: Add unit tests for org.apache.solr.util.TimeOut #3026

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sandbergja
Copy link
Contributor

This class did not appear to have any unit tests yet.

https://issues.apache.org/jira/browse/SOLR-17618

Description

Adding some unit tests for org.apache.solr.util.TimeOut, which did not have unit tests previously (although I believe the tests exercise it in other ways).

Solution

Adding a few simple tests

Tests

The attached unit tests pass for me, no functionality changes are included in this pull request.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

@github-actions github-actions bot added the tests label Jan 11, 2025
@sandbergja sandbergja force-pushed the timeout-tests branch 2 times, most recently from 492fe05 to 46da013 Compare January 11, 2025 22:59
@sandbergja sandbergja marked this pull request as ready for review January 11, 2025 23:00
This class did not appear to have any unit tests yet.
Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@madrob madrob left a comment

Choose a reason for hiding this comment

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

Thanks for this, some minor comments.

solr/core/src/test/org/apache/solr/util/TimeOutTest.java Outdated Show resolved Hide resolved
}

public void testHasTimedOut() {
TimeSource mockTimeSource = mock(TimeSource.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

This could go in a Before method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh, good point! Thanks, @madrob !

Copy link
Contributor

Choose a reason for hiding this comment

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

I would do Before, not BeforeClass for it, and also put the when/thenReturn line in there as well.

Then in the tests adjust the timeouts to test various combinations if we have the same mock time source each time. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay! I think I get it now. Thanks for working with me on this @madrob !

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

Successfully merging this pull request may close these issues.

3 participants