-
Notifications
You must be signed in to change notification settings - Fork 44
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
QPID-7615 Number of selectors can be equal to connection thread pool size #34
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to split positive and negative tests into separate methods.
The positive tests can be strengthened with asserts verifying attributes values
@@ -509,31 +509,47 @@ public void testExistingConnectionBlocking() | |||
} | |||
|
|||
@Test | |||
public void testCreateValidation() | |||
public void testSelectorNumberMustBePositiveOnCreate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to suggest splitting positive and negative tests into separate methods
{ | ||
createVirtualHost(getTestName(), Collections.singletonMap(QueueManagingVirtualHost.NUMBER_OF_SELECTORS, "1")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It make sense to verify that attribute "numberOfSelectors" is indeed set to the specified value, for example
Map<String, Object> attributes = Collections.singletonMap(QueueManagingVirtualHost.NUMBER_OF_SELECTORS, "1"); final QueueManagingVirtualHost<?> vh = createVirtualHost(getTestName(), attributes); assertEquals(1, vh.getNumberOfSelectors());
} | ||
|
||
@Test | ||
public void testConnectionThreadPoolSizeMustBePositiveOnCreate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, split method into positive and negative tests
final Map<String, Object> vhAttributes = new HashMap<>(); | ||
vhAttributes.put(QueueManagingVirtualHost.CONNECTION_THREAD_POOL_SIZE, 1); | ||
vhAttributes.put(QueueManagingVirtualHost.NUMBER_OF_SELECTORS, 1); | ||
createVirtualHost(getTestName(), vhAttributes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test can be strengthen with asserts verifying that specified values are indeed set on the corresponding attributes
} | ||
|
||
@Test | ||
public void testSelectorNumberMustBeLessOrEqualToThePoolSizeOnCreate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to suggest splitting positive and negative tests into separate methods
{ | ||
QueueManagingVirtualHost<?> virtualHost = createVirtualHost(getTestName()); | ||
final QueueManagingVirtualHost<?> virtualHost = createVirtualHost(getTestName()); | ||
virtualHost.setAttributes(Collections.singletonMap(QueueManagingVirtualHost.NUMBER_OF_SELECTORS, "1")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test can be strengthen with asserts verifying that specified values are indeed set on the corresponding attributes
} | ||
|
||
@Test | ||
public void testConnectionThreadPoolSizeMustBePositiveOnChange() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to suggest splitting positive and negative tests into separate methods
final Map<String, Object> vhAttributes = new HashMap<>(); | ||
vhAttributes.put(QueueManagingVirtualHost.CONNECTION_THREAD_POOL_SIZE, 1); | ||
vhAttributes.put(QueueManagingVirtualHost.NUMBER_OF_SELECTORS, 1); | ||
virtualHost.setAttributes(vhAttributes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test can be strengthen with asserts verifying that specified values are indeed set on the corresponding attributes
} | ||
|
||
@Test | ||
public void testSelectorNumberMustBeLessOrEqualToThePoolSizeOnChange() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to suggest splitting positive and negative tests into separate methods
{ | ||
final QueueManagingVirtualHost<?> virtualHost = createVirtualHost(getTestName()); | ||
virtualHost.setAttributes(Collections.singletonMap(QueueManagingVirtualHost.NUMBER_OF_SELECTORS, QueueManagingVirtualHost.DEFAULT_VIRTUALHOST_CONNECTION_THREAD_POOL_SIZE - 1)); | ||
virtualHost.setAttributes(Collections.singletonMap(QueueManagingVirtualHost.NUMBER_OF_SELECTORS, QueueManagingVirtualHost.DEFAULT_VIRTUALHOST_CONNECTION_THREAD_POOL_SIZE)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test can be strengthen with asserts verifying that specified values are indeed set on the corresponding attributes
Divided test testCreateValidation to
and test testChangeValidation to