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.

Sign up for GitHub

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

Jump to bottom

Move junit4 to assertj #5685

Merged
merged 20 commits into from
Aug 16, 2022
Merged

Move junit4 to assertj #5685

merged 20 commits into from
Aug 16, 2022

Conversation

eddumelendez
Copy link
Member

No description provided.

@eddumelendez eddumelendez added this to the next milestone Aug 13, 2022
kiview
kiview requested changes Aug 15, 2022
View reviewed changes
Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

Generally looks great. The only issue is the conversation of some assert description into the Assertj as(). I tried to eyeball them into something that makes more sense, but I was not super clear.

Here is the example from the docs:

// as() is used to describe the test and will be shown before the error message
assertThat(frodo.getAge()).as("check %s's age", frodo.getName()).isEqualTo(33);

So we should give them an additional look, with this background information.

I dedicate this PR review to @bsideup, whose effort in reviewing the last Assertj refactoring PR motivated me to go through this one file-by-file as well 😅

docs/examples/junit4/generic/src/test/java/generic/ExecTest.java Outdated Show resolved Hide resolved
docs/examples/junit4/generic/src/test/java/generic/HostPortExposedTest.java Outdated Show resolved Hide resolved
modules/cassandra/src/test/java/org/testcontainers/containers/CassandraContainerTest.java Outdated Show resolved Hide resolved
modules/cassandra/src/test/java/org/testcontainers/containers/CassandraDriver3Test.java Outdated Show resolved Hide resolved
modules/cassandra/src/test/java/org/testcontainers/containers/CassandraDriver4Test.java Outdated Show resolved Hide resolved
...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java Outdated
ffmpegOutput.contains("Duration: 00:") && !(ffmpegOutput.contains("Duration: 00:00:00.00"))
);
assertThat(ffmpegOutput.contains("Duration: 00:") && !(ffmpegOutput.contains("Duration: 00:00:00.00")))
.as("Duration is incorrect in:\n " + ffmpegOutput)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure, should contain the expected duration?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that would make the description more accurate

Copy link
Member

Choose a reason for hiding this comment

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

See my suggestion below. Feel free to accept, resolve this conversation and then merge 😇

...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java Outdated Show resolved Hide resolved
modules/trino/src/test/java/org/testcontainers/containers/TrinoContainerTest.java Outdated Show resolved Hide resolved
modules/trino/src/test/java/org/testcontainers/containers/TrinoContainerTest.java Outdated Show resolved Hide resolved
modules/trino/src/test/java/org/testcontainers/containers/TrinoContainerTest.java Outdated Show resolved Hide resolved
eddumelendez and others added 8 commits August 15, 2022 11:21
@eddumelendez @kiview
Update docs/examples/junit4/generic/src/test/java/generic/HostPortExp…
434c03c 
…osedTest.java

Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
@eddumelendez @kiview
Update modules/cassandra/src/test/java/org/testcontainers/containers/…
bf34364 
…CassandraDriver4Test.java

Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
@eddumelendez @kiview
Update modules/junit-jupiter/src/test/java/org/testcontainers/junit/j…
cc44e94 
…upiter/TestLifecycleAwareExceptionCapturingTest.java

Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
@eddumelendez @kiview
Update modules/presto/src/test/java/org/testcontainers/containers/Pre…
29a4495 
…stoContainerTest.java

Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
@eddumelendez @kiview
Update modules/presto/src/test/java/org/testcontainers/containers/Pre…
fb98147 
…stoContainerTest.java

Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
@eddumelendez @kiview
Update modules/presto/src/test/java/org/testcontainers/containers/Pre…
1a67fc1 
…stoContainerTest.java

Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
@eddumelendez
Fix
2348a25
@eddumelendez
Fix assertion
cdedbed
bsideup
bsideup reviewed Aug 15, 2022
View reviewed changes
config/checkstyle/checkstyle.xml
@@ -29,11 +29,7 @@
org.assertj.core.api.Assertions.*,
org.assertj.core.api.Assumptions.*,
org.awaitility.Awaitility.*,
org.hamcrest.Matchers.*,
org.hamcrest.MatcherAssert.*,
org.junit.Assert.*,
org.junit.Assume.*,
Copy link
Member

Choose a reason for hiding this comment

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

we should consider migrating this one to Assumptions too btw

bsideup
bsideup approved these changes Aug 15, 2022
View reviewed changes
Copy link
Member

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

💯 a few nits re automatically generated code, but excited to see these being replaced with AssertJ 🎉

eddumelendez and others added 2 commits August 15, 2022 19:55
@eddumelendez
Fix assertions
c5d0cad
@eddumelendez @kiview
Update modules/trino/src/test/java/org/testcontainers/containers/Trin…
935df82 
…oContainerTest.java

Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
eddumelendez and others added 6 commits August 15, 2022 22:29
@eddumelendez @kiview
Update modules/trino/src/test/java/org/testcontainers/containers/Trin…
01c6544 
…oContainerTest.java

Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
@eddumelendez @kiview
Update modules/selenium/src/test/java/org/testcontainers/junit/Chrome…
e629905 
…RecordingWebDriverContainerTest.java

Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
@eddumelendez @kiview
Update modules/trino/src/test/java/org/testcontainers/containers/Trin…
65ebf56 
…oContainerTest.java

Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
@eddumelendez @kiview
Update modules/cassandra/src/test/java/org/testcontainers/containers/…
ce983bd 
…CassandraDriver3Test.java

Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
@eddumelendez
Fix assertions
e0c929d
@eddumelendez
Merge branch 'master' into assertj2
63a2c0a
kiview
kiview reviewed Aug 16, 2022
View reviewed changes
...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java Outdated Show resolved Hide resolved
...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java Outdated
ffmpegOutput.contains("Duration: 00:") && !(ffmpegOutput.contains("Duration: 00:00:00.00"))
);
assertThat(ffmpegOutput.contains("Duration: 00:") && !(ffmpegOutput.contains("Duration: 00:00:00.00")))
.as("Duration is incorrect in:\n " + ffmpegOutput)
Copy link
Member

Choose a reason for hiding this comment

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

See my suggestion below. Feel free to accept, resolve this conversation and then merge 😇

@eddumelendez @kiview
Update modules/selenium/src/test/java/org/testcontainers/junit/Chrome…
ae944fb 
…RecordingWebDriverContainerTest.java

Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
@eddumelendez eddumelendez merged commit 6a3e649 into master Aug 16, 2022
@delete-merged-branch delete-merged-branch bot deleted the assertj2 branch August 16, 2022 14:25
@kiview kiview removed the type/docs label Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kiview kiview kiview approved these changes

@bsideup bsideup bsideup approved these changes

@rnorth rnorth Awaiting requested review from rnorth

Assignees
No one assigned
Labels
modules/cassandra modules/couchbase modules/influx modules/mongodb modules/solr type/housekeeping
Projects
None yet
Milestone
1.17.4
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@eddumelendez @bsideup @kiview