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

[test] Removes jmockit in favor of mockito #5063

Merged
merged 4 commits into from
Jan 22, 2020

Conversation

jimschubert
Copy link
Member

@jimschubert jimschubert commented Jan 21, 2020

We use mockito in many tests. This removes jmockit which is run as a javaagent in favor of Mockito which is not.

This work is in preparation for applying some static analysis tools, while evaluating others such as Jacoco. I'm also look at ways to improve build times while also decreasing "ramp up time" for contributions from the community. Reducing the number of mock frameworks and dependencies is a step toward that goal.

I'm opening this for discussion. It seems like jmockit was used heavily in GenerateTest in the CLI to test the internals of the generate task. I think this is the wrong approach; each component of that task should be tested in isolation.

I think this approach cleans up the options tests, as it was never really clear to me and most likely to others how to use the Expectations API in jmockit. I also found while testing that the Expectations of jmockit would silently swallow exceptions. For example, scala-http-client defines this method:

public void setModelPropertyNaming(String naming) {
    if ("original".equals(naming) || "camelCase".equals(naming) ||
            "PascalCase".equals(naming) || "snake_case".equals(naming)) {
        this.modelPropertyNaming = naming;
    } else {
        throw new IllegalArgumentException("Invalid model property naming '
                naming + "'. Must be 'original', 'camelCase', " +
                "'PascalCase' or 'snake_case'");
    }
}

The expectation in the test passed modelPropertyNaming as the option and verified that it was executed once. That execution should have resulted in an IllegalArgumentException on invocation, but jmockit's Expectation only cared that the method was called and hid that it was called with invalid data.

I plan to look at replacing the GenerateTest tests with unit tests, and whether it would make sense to construct the Generate class to evaluate argument parsing. The generate task now evaluates that CodgenConfigurator is called as expected. The DefaultGenerator implementation should be tested separately, considering previous tests weren't evaluating anything that was generated.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @OpenAPITools/generator-core-team

We use mockito in many tests. This removes jmockit which is run as a
javaagent in favor of Mockito which is not.

This work is in preparation for applying some static analysis tools,
while evaluating others such as Jacoco. I'm also look at ways to improve
build times while also decreasing "ramp up time" for contributions from
the community. Reducing the number of mock frameworks and dependencies
is a step toward that goal.
}

@Test
public void testLibrary() throws Exception {
final String value = "library1";
Copy link
Member Author

Choose a reason for hiding this comment

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

library1 here should have resulted in an exception, but jmockit swallowed it.

@@ -29,7 +29,7 @@
public static final String SORT_PARAMS_VALUE = "false";
public static final String SORT_MODEL_PROPERTIES_VALUE = "false";
public static final String ENSURE_UNIQUE_PARAMS_VALUE = "true";
public static final String MODEL_PROPERTY_NAMING = "modelPropertyNaming";
Copy link
Member Author

Choose a reason for hiding this comment

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

This old value should have resulted in an exception, but jmockit swallowed it.

@jimschubert jimschubert requested review from wing328 and jmini January 22, 2020 03:14
@jimschubert
Copy link
Member Author

fyi for @OpenAPITools/openapi-generator-collaborators

I'm going to go ahead and merge this as there are usually very few new/updated "Options" tests. Everything else seems to already use mockito.

@jimschubert jimschubert merged commit ac528aa into OpenAPITools:master Jan 22, 2020
@jimschubert jimschubert deleted the standardize-on-mockito branch January 22, 2020 23:04
@wing328
Copy link
Member

wing328 commented Jan 23, 2020

@jimschubert fine with mockito. Thanks for the effort 👍

@wing328 wing328 added this to the 4.2.3 milestone Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants