Spring-boot: Migrate from ExpectedException rule to AssertJ

[philwebb]

We should consider migrating from @ExpectedException to AssertJ. This will help if/when we upgrade to JUnit 5.

[ivange94]

Hi @philwebb I'm new here and would love to tackle this. Just so I'm clear, this issue involves modify all test that use @ExpectedException to a corresponding AssertJ alternative right? That is, this has no effect on the way users will interact with the framework right?

[snicoll]

@ivange94 thank you very much for offering to help. You are correct, the purpose here is to remove our use of ExpectedException in benefit of the AssertJ equivalent.

[ivange94]

@snicoll thanks for the quick response. I'm proceeding with working on this.

[ivange94]

When I build a fresh clone with ./mvnw clean install I get a test failure

[INFO] Running org.springframework.boot.SpringApplicationTests
[ERROR] Tests run: 75, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1,117.157 s <<< FAILURE! - in org.springframework.boot.SpringApplicationTests
[ERROR] failureResultsInSingleStackTrace(org.springframework.boot.SpringApplicationTests)  Time elapsed: 6.018 s  <<< FAILURE!
org.junit.ComparisonFailure: [Expected single stacktrace] expected:<[1]> but was:<[0]>
    at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
    at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
    at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
    at org.springframework.boot.SpringApplicationTests.failureResultsInSingleStackTrace(SpringApplicationTests.java:1168)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
    at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
    at org.springframework.boot.testsupport.rule.OutputCapture$1.evaluate(OutputCapture.java:57)
    at org.junit.rules.ExpectedException$ExpectedExceptionStatement.evaluate(ExpectedException.java:239)
    at org.junit.rules.RunRules.evaluate(RunRules.java:20)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:365)
    at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:273)
    at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:238)
    at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:159)
    at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:383)
    at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:344)

And when I try running the tests from the IDE (InteliJ Idea) I get

Error:java: Bad service configuration file, or exception thrown while constructing Processor object: javax.annotation.processing.Processor: Provider org.springframework.boot.configurationprocessor.ConfigurationMetadataAnnotationProcessor not found

I've done some research about the Idea errors and solutions that I see require me to make some modifications to the project so I guess I'm looking at the wrong places since all these must be working for everyone else.

[philwebb]

@ivange94 What operating system do you use?

[ivange94]

@philwebb macOS Sierra (10.12.6)

[philwebb]

@ivange94 That should work find. What does java -version and echo $JAVA_HOME show?

[ivange94]

java -version shows

Ivanges-MacBook-Pro:~ ivange$ java -version
java version "1.8.0_181"
Java(TM) SE Runtime Environment (build 1.8.0_181-b13)
Java HotSpot(TM) 64-Bit Server VM (build 25.181-b13, mixed mode)

JAVA_HOME is not set.

[philwebb]

@ivange94 I'm at a bit of a loss. You can try setting a JAVA_HOME but I don't think it will fix the test failure you're seeing. I'm not really sure what to suggest at this point.

[ivange94]

@philwebb it didn't.

[snicoll]

@ivange94 Bummer. Perhaps you could join us on Gitter and we can try to figure out what's going on on your machine?

[ivange94]

@snicoll that sounds great. Didn't know there was a gitter channel. How can I join please?

[ivange94]

@snicoll found the channel. We'll continue the discussion there.

[ivange94]

There are more than a thousand usage of ExpectedException in the codebase. Do I refactor the whole code base before submitting a PR or can I submit small PRs? I've already finished refactoring the spring-boot subproject inside spring-boot-project and this is already 66 files modified with 425 additions and 855 deletions.

I still have more than 12 subprojects to refactor.

[wilkinsona]

Thanks for taking a look at this, @ivange94. Before going any further, we should settle on the syntax that we want to use. AssertJ offers two choices:

@Test
public void testException() {
   assertThatThrownBy(() -> { throw new Exception("boom!"); }).isInstanceOf(Exception.class)
                                                             .hasMessageContaining("boom");
}

@Test
public void testException() {
   assertThatExceptionOfType(IOException.class).isThrownBy(() -> { throw new IOException("boom!"); })
                                               .withMessage("%s!", "boom")
                                               .withMessageContaining("boom")
                                               .withNoCause();
}

On first impressions, I think the second options reads better. Let's see what the rest of the team thinks so that we can hopefully reach agreement on which to use.

[mbhave]

+1 for assertThatExceptionOfType. The javadoc also says it's more or less the same as assertThatThrownBy but in a more natural way.

[philwebb]

Same. I like assertThatExceptionOfType.

[philwebb]

There are more than a thousand usage of ExpectedException

I wonder if we can script the migration somehow? Perhaps we can use the AST transform support from Atomist.

[ivange94]

I also found assertThatExceptionOfType felt more natural and that's what I've been using.

[ivange94]

I wonder if we can script the migration somehow? Perhaps we can use the AST transform support from Atomist.

@philwebb Automation sounds great but I'm not sure I know exactly what you mean by AST transform support. AssertJ authors created a migration script but the reason I didn't think of using it was because the script may end up changing a lot of stuffs instead of just the ExpectedException. Can you throw some light to your approach?

[philwebb]

The docs are a bit light, and I've not played with it yet but Atomist has some clever features where it can edit files based on the code structure rather than raw text. AST stands for abstract syntax tree, it basically means you can change things based on the parsed text, rather than the text itself.

Most of our tests look something like this:

    @Test
    public void createWhenEndpointTypeIsNullShouldThrowException() {
        this.thrown.expect(IllegalArgumentException.class);
        this.thrown.expectMessage("EndpointType must not be null");
        new ExposeExcludePropertyEndpointFilter<>(null, new MockEnvironment(), "foo");
    }

We'd need to extract IllegalArgumentException.class and "EndpointType must not be null" from the this.thrown calls then make the rest of the method a lambda and somehow rework it to this:

    @Test
    public void createWhenEndpointTypeIsNullShouldThrowException() {
        assertThatExceptionOfType(IllegalArgumentException.class).isThrownBy(()-> {
            new ExposeExcludePropertyEndpointFilter<>(null, new MockEnvironment(), "foo");
        }).withMessageContaining("EndpointType must not be null");
    }

[philwebb]

I'm new here and would love to tackle this

@ivange94 I very much appreciate the offer of help, but this is potentially quite a large and time consuming change. Please carry on investigating if you have the time, but also feel free to take a look at the ideal-for-contribution status label if you want to start with something less challenging.

[ivange94]

@philwebb you're right, this is a time consuming change. It gets boring doing the same thing over and over again. I'll look at the label but will still continue working on this. If can get the script working fine else I'll do it manually.

[philwebb]

@ivange94 Thanks for the offer to help but I've managed to migrate the tests now using a combination of search/replace, regex, refactoring, and sheer willpower :)