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.

Sign up for GitHub

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

Jump to bottom

DataProvider: possibility to unload dataprovider class, when done with it #2739

Merged
merged 1 commit into from Apr 21, 2022
Merged

DataProvider: possibility to unload dataprovider class, when done with it #2739

merged 1 commit into from Apr 21, 2022

Conversation

dsankouski
Copy link
Contributor

@dsankouski dsankouski commented Mar 10, 2022 •
edited

With current Data providers implementation, it's code will stick around
in method area (JVM spec $2.5.4) for entire test run.

By specifying dataprovider class with it's full qualified name, and
by using new custom classloader to load it, when needed, JVM gets a
chance to unload dataprovider class, when we're done and deleted
all links to it

Fixes #2724 .

Did you remember to?

  • Add test case(s)
  • Update CHANGES.txt
  • Auto applied styling via ./gradlew autostyleApply

@krmahadevan
Copy link
Member

@dsankouski - There are failures because the code style was not applied

Please Run ./gradlew autostyleApply to fix the violations and push in the changes

krmahadevan
krmahadevan reviewed Mar 11, 2022
View reviewed changes
Copy link
Member

@krmahadevan krmahadevan left a comment

Choose a reason for hiding this comment

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

@dsankouski - I still have the basic doubt around the following in this PR:

  1. When I provide a string version of my class as my data provider class, does it mean that the class exists in my class path but I am just not loading it before hand and will trigger the loading only when I run the data driven test which uses this class?

testng-core/src/main/java/org/testng/internal/DataProviderLoader.java Outdated Show resolved Hide resolved
testng-core/src/main/java/org/testng/internal/Parameters.java Outdated Show resolved Hide resolved
testng-core/src/main/java/org/testng/internal/Parameters.java Outdated Show resolved Hide resolved
testng-core/src/main/java/org/testng/internal/DataProviderMethodRemovable.java Show resolved Hide resolved
testng-core/src/main/java/org/testng/internal/annotations/JDK15AnnotationFinder.java Show resolved Hide resolved
testng-core/testng-core-build.gradle.kts Outdated Show resolved Hide resolved
testng-core/testng-core-build.gradle.kts Outdated Show resolved Hide resolved
krmahadevan
krmahadevan reviewed Mar 11, 2022
View reviewed changes
testng-core/src/main/java/org/testng/internal/Parameters.java Outdated
&& !dp.getDataProviderDynamicClass().isEmpty();
if (isDynamicDataProvider) {
try {
dataProviderClass = new DataProviderLoader()
Copy link
Member

Choose a reason for hiding this comment

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

We are creating on the fly instances of DataProviderLoader() to load the class that was specified as a string attribute in the @Test annotation. This means that this object of DataProviderLoader() immediately becomes eligible for a GC. So if the loader object gets GC'ed then all the classes that it caused to load would be unloaded as well.

So what is going to be the expected behaviour when the DataProviderLoader object got GC'ed even before a data provider was actually invoked? Wouldn't that cause ClassNotFoundException or NoClassDefFoundError for the test case which is using a dynamic data provider ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Class holds a reference to it's classloader. So, in order to garbage collect a classloader, all class instances should be collected first.

Copy link
Member

Choose a reason for hiding this comment

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

We are clearing only the instance. So does it mean that when all instances associated with a class are GC'ed the corresponding class also would get GC'ed from the perm space and thus cause the ClassLoader also to do the same ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right.

@dsankouski dsankouski force-pushed the dynamic_loading_dataprovider_class branch 2 times, most recently from 98475c7 to a0b7229 Compare March 12, 2022 15:44
@dsankouski dsankouski force-pushed the dynamic_loading_dataprovider_class branch from 56f0713 to 2112e44 Compare March 28, 2022 16:55
@krmahadevan
Copy link
Member

@dsankouski - My bad for getting back late on this. Can you please help fix the merge conflicts and update this PR branch? Lets work on getting this merged asap.

@dsankouski dsankouski force-pushed the dynamic_loading_dataprovider_class branch from 2112e44 to 8bdc42b Compare April 20, 2022 14:07
@dsankouski dsankouski changed the title WIP: DataProvider: possibility to dynamically load dataprovider class DataProvider: possibility to unload dataprovider class, when done with it Apr 20, 2022
juherr
juherr requested changes Apr 21, 2022
View reviewed changes
Copy link
Member

@juherr juherr left a comment

Choose a reason for hiding this comment

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

The new tests are failling on j9 jvm. They should be fixed or skipped when j9 is used.

@krmahadevan
Copy link
Member

@dsankouski

Can we please have this fixed ? https://github.com/cbeust/testng/runs/6096462677?check_suite_focus=true

org.testng.dataprovider.DynamicDataProviderLoadingTest > testDynamicDataProviderUnloaded failure marker
  Error:   0.2sec org.testng.dataprovider.DynamicDataProviderLoadingTest > testDynamicDataProviderUnloaded
  java.lang.IllegalArgumentException: javax.management.InstanceNotFoundException: com.sun.management:type=HotSpotDiagnostic
      at java.management/java.lang.management.ManagementFactory.newPlatformMXBeanProxy(ManagementFactory.java:623)
      at test.SimpleBaseTest$Companion.saveMemDump(SimpleBaseTest.kt:392)
      at org.testng.dataprovider.DynamicDataProviderLoadingTest.testDynamicDataProviderUnloaded(DynamicDataProviderLoadingTest.kt:40)
      Caused by: javax.management.InstanceNotFoundException: com.sun.management:type=HotSpotDiagnostic
          at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.getMBean(DefaultMBeanServerInterceptor.java:1083)
          at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.isInstanceOf(DefaultMBeanServerInterceptor.java:1379)
          at java.management/com.sun.jmx.mbeanserver.JmxMBeanServer.isInstanceOf(JmxMBeanServer.java:1091)
          at java.management/java.lang.management.ManagementFactory.isInstanceOf(ManagementFactory.java:651)
          at java.management/java.lang.management.ManagementFactory.newPlatformMXBeanProxy(ManagementFactory.java:611)
          ... 2 more

DataProvider: possibility to unload dataprovider class, when done wit…
c1c74b6 
…h it

With current Data providers implementation, it's code will stick around
in method area (JVM spec $2.5.4) for the entire test run.

By specifying dataprovider class with it's full qualified name, and
by using new custom classloader to load it, when needed, JVM gets a
chance to unload dataprovider class, when we're done with it.

Testing dataprovider class unload is performed by analysing memory dumps.
Also, there is a test(comparePerformanceAgainstCsvFiles) to measure performance
of data as code approach against common data approch, where data is stored in
csv files.
@dsankouski dsankouski force-pushed the dynamic_loading_dataprovider_class branch from 8bdc42b to c1c74b6 Compare April 21, 2022 07:29
@dsankouski
Copy link
Contributor Author

@krmahadevan @juherr, throw SkipException, in saveMemDump method, if jvm is not HotSpot

@krmahadevan krmahadevan merged commit aae0724 into testng-team:master Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juherr juherr juherr approved these changes

@krmahadevan krmahadevan krmahadevan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test data as code feature concept
3 participants
@dsankouski @krmahadevan @juherr