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

implement "parallelism-safe" Yatspec results generation, and fix memory leak #54

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pobrelkey
Copy link

Yatspec's HtmlIndexRenderer doesn't currently support test runs that occur across multiple JVM's - for example, builds that seek to improve performance by simultaneously running tests in several JVMs in parallel (as in Surefire with forkCount > 1), or builds that take an expedient approach to combating resource leaks by killing and re-spawning their test runner every so often (as in Gradle with forkEvery > 0).

This is because HtmlIndexRenderer only knows about tests run within its own JVM - more complex builds such as those described above would need some mechanism for passing data about test results between JVMs for index generation to work properly. This PR implements such a mechanism:

  • Two new interfaces are introduced: ResultMetadata, which provides a high-level subset of test result data sufficient for HtmlIndexRenderer and other classes that generate "summary"/"index" reports; and SpecResultMetadataListener, which a class can implement to receive this data about completed tests. HtmlIndexRenderer is refactored to implement the latter interface.
    • Why the new interfaces instead of continuing to use Result and SpecResultListener?
      • Serializing full Results between JVMs would require that whatever people put in their captured values be Serializable, which is a big ask/would break backward compatibility.
      • If HtmlIndexRenderer only holds onto the data it needs - and not captured values/spec text/other details which can run to hundreds/thousands of K per test for complex interaction tests - we've fixed a memory leak.
    • User-written summary report generators can now use these interfaces as well, and reap similar benefits - we've done this for a number of our custom report writers
  • When a test is completed, we serialize ResultMedatata for the test to the directory specified in the yatspec.data.dir system property, using plain old Java serialization (so no additional library dependencies on Jackson/XStream/whatever). We also check this directory for any test results which may have appeared since the last test was completed, and if any are found, feed these to any SpecResultMetadataListeners which have been registered.
    • This behavior requires that the yatspec.data.dir system property has a value - if this isn't specified, this mechanism doesn't operate, and Yatspec behaves as before.
    • This of course will require builds to clean/delete yatspec.data.dir between runs, lest results from previous runs show up in any generated reports.
    • Note that only SpecResultMetadataListeners, and not SpecResultListeners, get called by this mechanism - the idea being that each JVM should be responsible for writing the main result files about its tests, but index files should be a collective responsibility.

Using this improved Yatspec has allowed us to use parallelism for the first time in some of our acceptance test builds, in one case improving the time of a notoriously long-running build by 40%. It's also improved index coverage in other builds which spawn multiple runners, to the point where these have become usable for the first time.

Note that to maintain backwards source compatibility with existing Yatspec uses these changes utilize default interface methods and therefore will require Java 8.

@pobrelkey pobrelkey force-pushed the parallelism-upstream-pr branch from 4379cad to 0d561c7 Compare June 10, 2021 18:12
@pobrelkey pobrelkey marked this pull request as ready for review June 10, 2021 18:17
@pobrelkey pobrelkey force-pushed the parallelism-upstream-pr branch from 0d561c7 to a3654ae Compare July 2, 2021 16:31
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.

1 participant