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

[JENKINS-57928] Allow configuring JMH benchmarks using CASC #921

Merged
merged 12 commits into from
Jun 18, 2019

Conversation

AbhyudayaSharma
Copy link
Member

Your checklist for this pull request

🚨 Please review the guidelines for contributing to this repository.

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!

  • Please describe what you did

  • Link to relevant GitHub issues or pull requests

  • Link to relevant Jenkins JIRA issues

  • Did you provide a test-case? That demonstrates feature works or fixes the issue.

Description

Adds support for running JMH benchmarks with temporary instances configured using Configuration as Code. This is a direct follow-up to jenkinsci/jenkins-test-harness#135.

JIRA issue: https://issues.jenkins-ci.org/browse/JENKINS-57928

@jenkinsci/gsoc2019-role-strategy

@AbhyudayaSharma
Copy link
Member Author

I'm not sure about the Codacy review. The test method will throw an error if the code doesn't behave properly so I don't think there is any need for an assert or a fail().

Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

LGTM

@AbhyudayaSharma AbhyudayaSharma marked this pull request as ready for review June 14, 2019 05:31
@AbhyudayaSharma AbhyudayaSharma changed the title [JENKINS-57928] Allow configuring JMH benchmarks using CASC 🚧 [JENKINS-57928] Allow configuring JMH benchmarks using CASC Jun 14, 2019
@AbhyudayaSharma AbhyudayaSharma changed the title [JENKINS-57928] Allow configuring JMH benchmarks using CASC [JENKINS-57928] Allow configuring JMH benchmarks using CASC 🚧 Jun 14, 2019
* @throws ConfiguratorException when unable to configure
*/
@Override
public void setup() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

for setup, I suggest passing this so the resource loader will work for the current benchmark 😓
I don't know if there is a way to do it inside JUnit / benchmark framework

This how it is used here:

* Reads a resource from the classpath to use in asserting expected export content.
* <p>
* The resource is expected to be UTF-8 encoded.
* <p>
* Example usage:
* <pre>{@code
* toStringFromYamlFile(this, "expected-output.yml");}</pre>
*
* @param clazz pass in {@code this}
* @param resourcePath the file name to read, should be in the same package as your test class in resources
* @return the string content of the file
* @throws URISyntaxException invalid path
* @throws IOException invalid path or file not found in general
*/
public static String toStringFromYamlFile(Object clazz, String resourcePath) throws URISyntaxException, IOException {
URL resource = clazz.getClass().getResource(resourcePath);
if (resource == null) {
throw new FileNotFoundException("Couldn't find file: " + resourcePath);
}
byte[] bytes = Files.readAllBytes(Paths.get(resource.toURI()));
return new String(bytes, StandardCharsets.UTF_8).replaceAll("\r\n?", "\n");
}

@AbhyudayaSharma AbhyudayaSharma changed the title [JENKINS-57928] Allow configuring JMH benchmarks using CASC 🚧 [JENKINS-57928] Allow configuring JMH benchmarks using CASC Jun 14, 2019
@AbhyudayaSharma
Copy link
Member Author

AbhyudayaSharma commented Jun 14, 2019

@Casz I've made it work though it's not too pretty. The problem is that we cannot pass an object to a benchmark as it is run in a different JVM. Also, the class that will extend CascJmhBenchmarkState would completely transformed when the benchmarks are run, so we cannot use getClass().getEnclosingClass().

Also, I don't know why the Travis builds are failing. Reflections should be present as it is a dependency of Jenkins Test Harness now.

String config = Objects.requireNonNull(getClass().getClassLoader().getResource(getResourcePath())).toExternalForm();
Class<?> enclosingClass = getEnclosingClass();

if (!enclosingClass.isAnnotationPresent(JmhBenchmark.class)) {
Copy link
Member

Choose a reason for hiding this comment

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

Neat! 👍

@Nonnull
@Override
protected Class<?> getEnclosingClass() {
return SampleBenchmark.class;
Copy link
Member

Choose a reason for hiding this comment

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

Might as well return this; or does that not work?

Copy link
Member Author

Choose a reason for hiding this comment

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

That doesn't work. Like I said above io.jenkins.plugins.casc.SampleBenchmark becomes io.jenkins.plugins.casc.generated.SampleBenchmark_MyState_jmhType

@AbhyudayaSharma
Copy link
Member Author

@Casz Is it ready for merging into master?

@jetersen
Copy link
Member

No CI and Codacy checks failed 😓

@AbhyudayaSharma
Copy link
Member Author

AbhyudayaSharma commented Jun 15, 2019

I need help here. The Jenkins build seems to fail just because the Docker container does not have enough space.

[2019-06-15T13:04:55.209Z] [ERROR] io.jenkins.plugins.casc.vault.VaultSecretSourceTest  Time elapsed: 2.348 s  <<< FAILURE!

[2019-06-15T13:04:55.209Z] java.lang.AssertionError: Docker environment should have more than 2GB free disk space

The Travis build on the other hand seems to have it's build cache corrupted. Reflections comes from Jenkins Test Harness now and should be present on the classpath. Any ideas?

@jetersen
Copy link
Member

Regarding Jenkins: https://issues.jenkins-ci.org/browse/INFRA-2062

Regarding Travis seems to resolve to a different version of Guava than that of Jenkins

@AbhyudayaSharma
Copy link
Member Author

Thanks @Casz ! Now what should I do about Travis?

@jetersen
Copy link
Member

ronmamo/reflections#219

This should be fixed in Jenkins test harness?

@jetersen
Copy link
Member

Or just say no to guava CC @oleg-nenashev

@oleg-nenashev
Copy link
Member

Yes, likely a JTH fix needed. Is there a suitable reflections version which is compatible with guava inside the core?

@AbhyudayaSharma
Copy link
Member Author

AbhyudayaSharma commented Jun 17, 2019

I have created a pull request to Jenkins Test Harness using ClassGraph SezPoz.

jenkinsci/jenkins-test-harness#141

@jetersen
Copy link
Member

@AbhyudayaSharma
Copy link
Member Author

AbhyudayaSharma commented Jun 17, 2019

@Casz Yes, thanks! I'm just waiting for tests on my machine to finish.

@jetersen
Copy link
Member

all green except for codacy 😆

…tateTest.java

Co-Authored-By: Joseph Petersen <josephp90@gmail.com>
@jetersen
Copy link
Member

Now just waiting on a release of JTH 😓

@AbhyudayaSharma
Copy link
Member Author

AbhyudayaSharma commented Jun 17, 2019

Oleg said in the Role Strategy Plugin Gitter chat that he will try to release a new JTH version (and plugin-pom) today.

@AbhyudayaSharma
Copy link
Member Author

Newer JTH was released: jenkinsci/plugin-pom#213

@jetersen jetersen merged commit 6311ebf into jenkinsci:master Jun 18, 2019
@AbhyudayaSharma AbhyudayaSharma deleted the jmh branch June 18, 2019 08:01
@AbhyudayaSharma
Copy link
Member Author

Thanks a lot @Casz ! Would we be getting a release soon?

@jetersen
Copy link
Member

Incremental should be available in the meantime 😅

@jetersen jetersen added the test A PR that adds to testing - used by Release Drafter label Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test A PR that adds to testing - used by Release Drafter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants