Skip to content

Add Script & Library Caching#748

Open
The-Jonsey wants to merge 1 commit intojenkinsci:masterfrom
The-Jonsey:feature/Add-Script-And-Library-Caching
Open

Add Script & Library Caching#748
The-Jonsey wants to merge 1 commit intojenkinsci:masterfrom
The-Jonsey:feature/Add-Script-And-Library-Caching

Conversation

@The-Jonsey
Copy link

Added a script and library cache to the PipelineTestHelper, this helped reduce the time a suite of tests took on a complex pipeline library from several minutes to ~30 seconds, as each test previous took ~7 seconds to load the script and the library. Any changes to mocks or script/libraries during test execution would be ignored by the caching, so the cache is disabled by default with a flag to enable it.

Testing done

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests that demonstrate the feature works or the issue is fixed

@The-Jonsey
Copy link
Author

@nre-ableton any chance you could give this a look?

Copy link
Contributor

@nre-ableton nre-ableton left a comment

Choose a reason for hiding this comment

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

First of all, thanks for your PR and sorry for the delay in reviewing. It was indeed in my GitHub notifications, but things have been quite busy at work recently (same old excuses 🙃).

Anyways, the concept of this PR seems fine, but I am not wild about the API. In general, variables/methods/classes/etc with And in the name should be an indication that they are trying to do too many things and should be refactored accordingly.

I would recommend the following:

  • Remove shouldCacheScriptsAndLibraries and replace with two distinct properties: cacheScripts and cacheLibraries
  • Eliminate the withShouldCacheScriptsAndLibraries method and simply allow users to set the above properties as usual
  • Use the respective fields when clearing cached classes/libraries

I have other minor suggestions, but I don't think they are worth discussing until we figure out the "big picture" here.

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.

2 participants