Conversation
OptimumCode
left a comment
OptimumCode
left a comment
There was a problem hiding this comment.
Could you please add a unit test for dynamic aliases?
| Timestamp startTime = toTimestamp(interval.getStartTime()); | ||
| Timestamp endTime = toTimestamp(interval.getEndTime()); | ||
|
|
||
| Collection<String> newAliases = null; |
There was a problem hiding this comment.
I would suggest to use Collection.emptySet() instead of null. It is always better to have the variable with not null value. Also, it might help to simplify the logic. For example, you can call sessionAliases.addAll(newAliases); right at the start and nothing will changes with sessionAliases
|
|
||
| Collection<String> newAliases; | ||
|
|
||
| do { |
There was a problem hiding this comment.
I think we should check if the aliases are not empty before making any requests. Because if aliases are empty there is no point to make any request to data provider
There was a problem hiding this comment.
I'm not sure if I have understood you correctly, can you please check out my last commit?
There was a problem hiding this comment.
I meant we should check the aliases retrieved from the data provider. The parameters verification should be done earlier in the constructor.
| Iterator<StreamResponse> iterator1 = new MessageSearchResponse(messages1).iterator(); | ||
| Iterator<StreamResponse> iterator2 = new MessageSearchResponse(messages2).iterator(); | ||
|
|
||
| when(manager.getDataProviderMock().searchMessages(any(MessageSearchRequest.class))).thenReturn(iterator1, iterator2); |
There was a problem hiding this comment.
I think you should configure stub more specific (expect the certain aliases in the search request)
There was a problem hiding this comment.
expect the certain aliases in the search request
What do you mean by that? To add a verify() call to check what aliases Crawler gets from each invocation of searchMessages()?
There was a problem hiding this comment.
I mean you should specify when mock should return each iterator. The current version will return the first iterator and then the second. It won't look at MessageSearchRequest parameters. I suggest you use argThat instead of any for mocking and check inside the argThatmethod which aliases were requested
|
|
||
| Collection<String> newAliases; | ||
|
|
||
| do { |
There was a problem hiding this comment.
I meant we should check the aliases retrieved from the data provider. The parameters verification should be done earlier in the constructor.
| verify(manager.getIntervalsWorkerMock()).getIntervals(any(Instant.class), any(Instant.class), anyString(), anyString(), anyString()); | ||
| verify(manager.getIntervalsWorkerMock()).storeInterval(any(Interval.class)); | ||
|
|
||
| verify(manager.getDataProviderMock(), times(2)).searchMessages(any(MessageSearchRequest.class)); |
There was a problem hiding this comment.
Shouldn't we check the alias parameter of the MessageSearchRequest object?
There was a problem hiding this comment.
I meant we should check the aliases retrieved from the data provider. The parameters verification should be done earlier in the constructor.
What kind of verification? In the constructor I already call method matchRequestedSessionAliases() that checks if sessionAliasPatterns is not null. If sessionAliases is empty, no request to the data-provider will be made due to this block:
if (crawlerType == DataType.MESSAGES && parameters.getSessionAliases().isEmpty()) { sendingReport = Report.empty(); break; }
There was a problem hiding this comment.
The parameters.getSessionAliases() might not be empty but the provided simply does not have aliases that match the specified parameters
There was a problem hiding this comment.
As far as I can see, parameters.getSessionAliases() will never contain aliases that do not match the specified parameters. In the constructor those aliases are set with new HashSet<>(matchRequestedSessionAliases()) that will check the aliases before returning them. Further, the aliases from parameters.getSessionAliases() are set with getNewAliases() method that calls matchRequestedSessionAliases() internally, too.
There was a problem hiding this comment.
I think we don't understand each other. I mean that the actual session aliases retrieved from the data provider might be empty (new HashSet<>(matchRequestedSessionAliases())). And we should check them instead parameters.getSessionAliases(). If the aliases retrieved from the data provider are empty there is no point in making a request to get messages/events.
There was a problem hiding this comment.
I think I got your point. I made a commit where I check sessionAliases before making any requests.
2 participants
No description provided.