Implement two separate builders for PullRequest and Repository #188
Conversation
| protected Mockable previousMock; | ||
|
|
||
| private static final Set<Mockable> requiredMockables; | ||
| static { |
There was a problem hiding this comment.
This section doesn't seem just right. Perhaps I can convert this into an injectable bean, which could based on a new annotation pick up all Mockable-s and fill the set this way. It would also not rely on static variables and context.
However, that might be a bit too overkill for testing purposes and this might be more readable and understandable for future reference.
Let me know, what you think @xstefank
There was a problem hiding this comment.
TBH, not sure if there should be a default at all. If this is used everywhere, it can be in the constructor.
There was a problem hiding this comment.
usually a new check or functionality always calls some new API. Thus a default mock needs to be there, otherwise it would throw many exceptions.
Example, adding check for config file always retrieves a file, thus every test will try to retrieve some file. We mock that no file is there to retrieve instead of NPE from mockito.
There was a problem hiding this comment.
It's static, as this method is called in tests annotated with BeforeAll, which has to be on static method. Thus static context is required.
| * Note: Only one {@code Mockable} per domain is allowed to be mocked. | ||
| * | ||
| * Parallel execution is not supported as we are relaying static variables. | ||
| */ |
There was a problem hiding this comment.
Comments go on the class declaration, not before package or imports
| protected Mockable previousMock; | ||
|
|
||
| private static final Set<Mockable> requiredMockables; | ||
| static { |
There was a problem hiding this comment.
TBH, not sure if there should be a default at all. If this is used everywhere, it can be in the constructor.
resolves #148