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

#860 switch to using assertj instead of FEST assertions #863

Merged
merged 2 commits into from Aug 27, 2016

Conversation

Projects
None yet
4 participants
@filiphr
Copy link
Member

filiphr commented Aug 25, 2016

This is a pull request for (#860) switching the assertions framework to the maintained assertj instead of FEST.

@mapstruct-robot

This comment has been minimized.

Copy link
Collaborator

mapstruct-robot commented Aug 25, 2016

(Message from the pull-request-builder): Admins, please verify this patch for it to build in the pull-request-builder.

exists();
isFile();

try {
return Assertions.assertThat( Files.toString( actual, Charsets.UTF_8 ) );
}
catch ( IOException e ) {
failIfCustomMessageIsSet( e );
//TODO shouldn't we just fail if something like this happens.

This comment has been minimized.

@filiphr

filiphr Aug 25, 2016

Author Member

@gunnarmorling Have a better look at this TODO. failIfCustomMessageIsSet does not exists in assertj anymore and I was not sure whether to directly fail with some message or do something else. Previously no custom message was set so even if an exception occurred the test did not fail.

This comment has been minimized.

@agudian

agudian Aug 25, 2016

Member

Good catch! Let's just use fail("Unable to read " + actual, e).

@agudian

This comment has been minimized.

Copy link
Member

agudian commented Aug 25, 2016

Awesome, @filiphr, thanks for the PR! Would you replace the TODO with the fail call? You can directly amand your commit or squash the change to force-push it to your branch.

@gunnarmorling

This comment has been minimized.

Copy link
Member

gunnarmorling commented Aug 25, 2016

Nice one, thanks a lot!

@filiphr

This comment has been minimized.

Copy link
Member Author

filiphr commented Aug 25, 2016

I am glad that you like the PR. I'll fix the TODO and do a force-push, tomorrow evening probably

@filiphr filiphr force-pushed the filiphr:swtich-to-assertj branch from 63d6f29 to b9ad8ac Aug 26, 2016

@filiphr

This comment has been minimized.

Copy link
Member Author

filiphr commented Aug 26, 2016

@agudian I replaced the TODO with a fail message

@gunnarmorling

This comment has been minimized.

Copy link
Member

gunnarmorling commented Aug 27, 2016

@filiphr I'm getting some failures in the integration tests, supposedly due to the requirement of JDK 8 by that new version (we run some integration tests on previous compiler versions). Didn't look into the details yet, but it seems we may actually have to use that version that run with JDK 7. Did you get the same when running mvn clean install on the entire project?

@gunnarmorling

This comment has been minimized.

Copy link
Member

gunnarmorling commented Aug 27, 2016

Jenkins, test this please.

@filiphr

This comment has been minimized.

Copy link
Member Author

filiphr commented Aug 27, 2016

@gunnarmorling I did mvn clean install on the entire project. I did it again and realized that it didn't run with oracle_java_6 and oracle_java_7 that is why I didn't see the errors. A solution would be to use the 1.x branch which is requires java 6 or above. 2.x is for java 7 or higher.

Is it an option to use 1.x only in the integration tests?

@filiphr

This comment has been minimized.

Copy link
Member Author

filiphr commented Aug 27, 2016

My last commit should fix the failing tests

@agudian

This comment has been minimized.

Copy link
Member

agudian commented Aug 27, 2016

ok to test

@agudian

This comment has been minimized.

Copy link
Member

agudian commented Aug 27, 2016

I think it's a good idea to use the Java 6 compatible version only in the integration tests. Focus there isn't on fancy assertions anyway, but on correct compilation using the different compilers.

@agudian

This comment has been minimized.

Copy link
Member

agudian commented Aug 27, 2016

Alright, the pull-request-builder looks healthy now as well. I'd say you squash the second commit again into the first one and I'll push it to master then... 👍

@agudian agudian merged commit cf88cf9 into mapstruct:master Aug 27, 2016

1 check passed

default Build finished.
Details
@agudian

This comment has been minimized.

Copy link
Member

agudian commented Aug 27, 2016

Never mind, forgot we have squash-and-merge for quite some time now here... :neckbeard:

@filiphr filiphr deleted the filiphr:swtich-to-assertj branch Nov 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.