OFBIZ-13311 Improved: Rewrite UtilCacheTest

[gtchaboussie]

(OFBIZ-13311)

Improved: Rewrites the test class UtilCacheTest.

Changes the variable names to make them more clear and moves the test utility objects to a dedicated new class.
Also removes the GenericTestCaseBase, that was used only in the UtilCacheTest.
Deleted a part of the testExpire test method, that was based on the useAllMemory method, which is highly OS or hardware dependant making unit test dependant as well.

Thanks: Nicolas for the help in analyzing.

[gtchaboussie]

Feedback on what was done in the task, and while re-writing

• I enabled the JUnit4 tests in gradle (that's debatable, but allows for the removal of all groovy test files to be run during tests)
• After concertation with Nicolas, removed the GenericTestBaseCase, that wasn't in framework or plugins
• I migrated the cache tests in groovy
• I separated the actuel tests from the differents utility methods and objects (and put them in a separated file)
• I removed the hamcrest dependency to use the vanilla groovy assertion
• I exploded the basicTest test method, and rewrote several test following the standard CRUD operations.
• I cleaned some of the assertions that were made in some of the tests
• Merged the part of basicTest related to expiration with the expireTest
  Obsiously, all of the above is open to discussion.

[jacopoc]

Hi Gaetan,
I didn’t review your pull request in detail because I don’t have deep knowledge of the original Java tests that you reimplemented in Groovy. However, I agree with the general direction and approach when dealing with legacy code: if there are opportunities to simplify old designs, it’s reasonable to introduce some simplifications, even at the cost of removing minor features, in order to achieve a more modern and maintainable codebase. In particular, I agree with your decision to remove the GenericTestCaseBase class and the useAllMemory method.

I do have one concern about useJUnit(), which, if I’m not mistaken, configures Gradle to use JUnit 4. I believe there are some Groovy tests that rely on JUnit 5, so this change might prevent them from running. More generally, it would be helpful to better understand the underlying reasons for the issues we encountered, as this knowledge could help us improve our testing framework setup.

Anyway, thanks for your work! In my opinion, we can accept this pull request if you feel confident about it.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[gtchaboussie]

Thanks for the feedback. Indeed, some of the test that are specified in the build.gradle file are not run anymore with junit enabled explicitely.
I'm lookling into it.

[gtchaboussie]

After some resarch, here is my analysis.
The following tests were explicitly added to groovy sources.

include 'org/apache/ofbiz/service/ModelServiceTest.groovy'
include 'org/apache/ofbiz/test/TestServices.groovy'
include 'org/apache/ofbiz/base/util/string/FlexibleStringExpanderBaseCodeTests.groovy'
include 'org/apache/ofbiz/base/util/FileUtilTests.groovy'
include 'org/apache/ofbiz/service/test/ServicePurgeTest.groovy'
include 'org.apache.ofbiz.base.test.SimpleTests.groovy'

ModelServiceTest.groovy, FlexibleStringExpanderBaseCodeTests.groovy, and FileUtilTests.groovy are indeed unit tests, and already are in relevant test sources. With junit enabled in build.gradle, they run and don't need the explicit declaration.
So they are ok.

TestServices.groovy is a groovy service file, and doesn't contain any test. It was most likely added by mistake.

As for ServicePurgeTest.groovy and SimpleTests.groovy, they look like integration tests.
They depend on the GroovyAssert class, that uses Junit5 indeed. But they don't need to be treated as source.
As it is in trunk, they are not run though. The declaration of both test suits has been deleted, and is not in any component anymore.

I suggest reenableing these tests in appropriate components (commented for now until they are validated) and keeping the simple junit architecture.
We can create an other task to sort out ServicePurgeTest.groovy and SimpleTests.groovy.

By the way, @JacquesLeRoux , do you remember the reason behind this commit that removed the test declaration ?

I'm not sure I understand why ServicePurgeTest.groovy and SimpleTests.groovy wouldn't be integration tests anymore

[jacopoc]

ModelServiceTest.groovy should depend on Junit 5 too

[gtchaboussie]

@jacopoc do you mean the test class should be an integration test ? Because it uses a dispatcher ?
Curently it uses mockito to emulate the dispatcher.

[JacquesLeRoux]

Hi Gaetan, Jacopo,

First, thanks Gaetan for your effort in this matter (tests), not so complex but sometimes kind of complicated. Thanks too Jacopo for your help.

When we worked on https://issues.apache.org/jira/browse/OFBIZ-12813, which was a massive task among related others (Minilang services and tests migration to Groovy, notably https://issues.apache.org/jira/browse/OFBIZ-12995), we missed some points. Great this part (tests) was reviewed.

About TestServices.groovy I'm torn after having a look and trying things. Also I already had a look at your question and stumbled upon issues. I'll get back to those points later...

[JacquesLeRoux]

@jacopoc do you mean the test class should be an integration test ? Because it uses a dispatcher ? Curently it uses mockito to emulate the dispatcher.

Actually, it seems to me that as soon as dispatcher or delegator is used the test should be considered an integration one, even if there is no real access to the DB (but then why using them ?). BTW, I wonder if ModelServiceTest.groovy should not extend GroovyScriptAssert.java. Maybe indeed the Mockito cas is particular.

[gtchaboussie]

Hi Jacques !

it seems to me that as soon as dispatcher or delegator is used the test should be considered an integration one

I agree with this.

I wonder if ModelServiceTest.groovy should not extend GroovyScriptAssert.java.

I also agree, and i think it is the indented use of the GroovyScriptAssert.java class. To allow easy integration tests to be written.

So this would give the following next actions / tasks :

• Reactivate ServicePurgeTest.groovy and SimpleTests.groovy
• Migrate ModelServiceTest.groovy to an integration test

[jacopoc]

@gtchaboussie I am referring to the fact that ModelServiceTest imports the org.junit.jupiter.api.* that was introduced with JUnit 5.

[JacquesLeRoux]

Sorry guys,

I did nothing after my message. I got a very weird "computer" issue today. Suddenly, "while doing nothing", the screen (a
recent simple Samsung one) got totally black, but a led on front lits up. I thought it was the end of my graphic cart (11 years). I bought one on Amazon to get it tmrw. Then I checked the screen. There is a button on the back at right. If you push on it the screen stops. I guess "while doing nothing" I did not realise that I pushed this button when adapting my keyboard to a new position. I don't like much Amazon, but you can cancel an order w/o paying if you are quick enough. Nothing better to challenge your brain, and find the real reason of an issue :D.

Of course in the middle of all that, I did some other things. It's quite stressing when you loose your screen!

[JacquesLeRoux]

Hi Gaetan,

it seems to me that as soon as dispatcher or delegator is used the test should be considered an integration one

I agree with this.

I tried to us SimpleTests.groovy as it was, a JUNIT simple test. It turned out to be impossible because of dispatcher that ultimately needs dispatcherFactory which is only assigned in ServiceContainer::init. Else SimpleTests::testTrue and SimpleTests::testDelegator worked.

So I converted SimpleTests.groovy to be an integration test. I have slightly modified the comments about that in build.gradle and will do the same in developer-manual.adoc where the OFBiz components main directory structure is documented.

I continue on the rest...

[JacquesLeRoux]

All tests in ModelServiceTest.groovy are OK as is (unit-tests).

About ServicePurgeTest.groovy, it's now an integration test as it should always have been.

TestServices.groovy is not a real test and should not have been considered as.

As I said the services and tests migration from Minilang to Groovy was a huge task; not surprised that few wrong cases escaped to us.

Here is attached OFBIZ-13311.patch which concludes my work. I'll also post it on the related Jira. I guess it's better to apply it after the rest on your side has been completed, just pick what you prefer to do :)

[JacquesLeRoux]

Hi Gaetan,

Mmm, something I forgot I not only changed TestServices.groovy but also moved it. So maybe testing it and even more committing it could be a problem with a simple patch. I suggest rather that I wait for your work here to be completed and then I'll push directly from my side, OK?

[gtchaboussie]

That's ok for me. I'll just push the CacheTest rewrite then, and let you contribute the other tests fixes.

[JacquesLeRoux]

👍

[gtchaboussie]

The tests don't run anymore. Investigating..
probably CKI (Chair Keyboard Interface).