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

ACS-5449 Bump mockito-core from 4.9.0 to 5.4.0 #2071

Merged

Conversation

viepovsky
Copy link
Contributor

@viepovsky viepovsky commented Jul 14, 2023

First failure was caused by CamelComponentsTestclass , there were problems with bean initialization from test-messaging-context.xml. Above mentioned class along with CamelRoutesTest class uses this xml file. These classes had annotation @Category(NeverRunsTests.class), and after adding @Ignore to deactivate them, now tests pass. (I tried many ways of fixing test-messaging-context.xml file but didn't find any solution, only @Ignore makes it pass).

Edit: As @damianujma found out, that we cannot delete these tests, I backed to work to find solution for failing tests.
Finally I have found a solution for these camel tests, the error which caused problems was : No qualifying bean of type 'org.alfresco.repo.rawevents.TransactionAwareEventProducer' available: expected at least 1 bean which qualifies as autowire candidate. Dependency annotations: {@org.springframework.beans.factory.annotation.Autowired(required=true)} and whatever I did it was not possible to mock certain classes (apart from TransactionAwareEventProducer class there were more exceptions for other mocked classes). Anyway after many tries, finally I inserted mocked beans on top of the application context xml file(test-messaging.xml) added type of argument and it does work now.
What caused the problem? Upgrading mockito-core from 4.9.0 to 4.10.0 and above causes this. I found one releted issue which I think is connected with this, as someone also had this problem, but they did not find solution. Anyway the solution is just putting mocked beans on the top of xml file before scanning spring components. Here is link to the issue: mockito/mockito#2779 (comment) , there is also mentioned that now when mocking and using xml, the type of argument is needed(it is also connected with the last failure with NodeLocatorServiceImplTest)

Second failure with access modifier, LockServicePoliciesImpl class was private, and after changing it to public there are no more IllegalAccessExceptions.

Third failure connected with RuleServiceImplUnitTest class, which tests RuleServiceImpl, this class has got private field SimpleCache<NodeRef, List<Rule>> nodeRulesCache;. And in RuleServiceImplUnitTest was field with raw interface without declaration just SimpleCache nodeRulesCache;, this caused problems in exception: java.lang.NullPointerException: Cannot invoke "org.alfresco.repo.cache.SimpleCache.remove(java.io.Serializable)" because "this.nodeRulesCache" is null.
After adding types to interface tests are passing.

The last failure was connected with NodeLocatorServiceImplTest class due to its bean initialization file test-nodeLocatorServiceImpl-context.xml, the problem was with recognizing proper overloaded method, I had to provide argument type (type=java.lang.Class), so the compiler could know which method to call.

Oskar Rajzner added 30 commits July 14, 2023 13:09
@dsibilio dsibilio self-requested a review July 21, 2023 09:52
@damianujma damianujma self-requested a review July 21, 2023 09:54
Copy link
Contributor

@dsibilio dsibilio left a comment

Choose a reason for hiding this comment

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

See https://github.com/Alfresco/alfresco-community-repo/actions/runs/5609730718/job/15198015845#step:7:474.

Thanks @damianujma for noticing that the Camel tests were actually being run anyways successfully before the Mockito upgrade... so we actually kind of need to maintain them and fix issues that come with the Mockito upgrade rather than removing the tests. The name for the NeverRunsTests category was misleading.

Copy link
Contributor

@damianujma damianujma left a comment

Choose a reason for hiding this comment

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

Even though the tests are "NeverRunsTest" annotated, these tests are run actually (e.g. https://github.com/Alfresco/alfresco-community-repo/actions/runs/5609730718/job/15198015845#step:7:474). I think we shouldn't remove them.

@viepovsky viepovsky requested review from tpage-alfresco and removed request for mikolajbrzezinski and tpage-alfresco July 21, 2023 11:36
@viepovsky viepovsky marked this pull request as draft July 21, 2023 11:41
@viepovsky viepovsky force-pushed the fix/ACS-5449_investigate-bump-mockito-core-test-failure branch from 59111fc to 4ee5c0d Compare July 21, 2023 12:05
@viepovsky viepovsky dismissed stale reviews from aonych and tpage-alfresco July 21, 2023 12:08

Still working on this

@viepovsky viepovsky force-pushed the fix/ACS-5449_investigate-bump-mockito-core-test-failure branch from 4ee5c0d to 85408bf Compare July 21, 2023 13:23
@viepovsky viepovsky marked this pull request as ready for review July 25, 2023 14:23
@viepovsky viepovsky merged commit 4faeaff into master Jul 26, 2023
76 checks passed
@viepovsky viepovsky deleted the fix/ACS-5449_investigate-bump-mockito-core-test-failure branch July 26, 2023 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants