Skip to content

Conversation

@JunHyungJang
Copy link
Contributor

Please let me know, if I should do something more or something to fix
Thank you

Check List:
Fixes AllOf should report which condition failed #3537
Unit tests : YES
Javadoc with a code example (on API only) : YES
PR meets the contributing guidelines
Following the contributing guidelines will make it easier for us to review and accept your PR.

Copy link
Member

@joel-costigliola joel-costigliola left a comment

Choose a reason for hiding this comment

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

thanks for the PR, first round of comments :)

import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
import static org.assertj.core.api.BDDAssertions.then;

public class AbstractAssert_satisfiesAllOf_Test {
Copy link
Member

Choose a reason for hiding this comment

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

Could you refactor that test so that it follows our contribution guidelines https://github.com/assertj/assertj/blob/3.x/CONTRIBUTING.md, especially the use of GIVEN WHEN THEN and AssertionUtil.expectAssertionError

@Test
void should_fail_if_one_of_the_condition_is_not_met() {
Condition<String> condition1 = new Condition<>(text -> text != null, "Input not null");
Condition<String> condition2 = new Condition<>(text -> text.equalsIgnoreCase("abc"), "Input not matching");
Copy link
Member

Choose a reason for hiding this comment

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

add another failing condition to show that all failing conditions are reported (and not the first failing)

@JunHyungJang
Copy link
Contributor Author

Thank you for your first round of comments !

  1. I add the testcase which fails every conditions
  2. I fix the testcase following the contributing guidlines

@joel-costigliola
Copy link
Member

Integrated thanks @JunHyungJang !

@injae-kim
Copy link

injae-kim commented Aug 4, 2024

image

Nice work @JunHyungJang !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants