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

[Feature] Bring SoftAssertions to Java #819

Open
uchagani opened this issue Feb 18, 2022 · 16 comments · Fixed by #1361
Open

[Feature] Bring SoftAssertions to Java #819

uchagani opened this issue Feb 18, 2022 · 16 comments · Fixed by #1361

Comments

@uchagani
Copy link
Contributor

Node version has soft assertions now. Can we get soft assertions for Java too?

@yury-s
Copy link
Member

yury-s commented Feb 18, 2022

That's certainly possible but the lack of good integrated test runner like the one in nodejs makes it less convenient in java, let's see if there is more interest in bringing this to java.

@uchagani
Copy link
Contributor Author

If we could integrate it with assertJ’s soft assertions that might be good enough

@NikkTod
Copy link

NikkTod commented Feb 23, 2022

+1 This is definitely something we should see in java and c#

@ankushsharma67
Copy link

+1, it should be brought to Java as well

@Sanady
Copy link

Sanady commented Nov 22, 2022

+1

@mecklena
Copy link

mecklena commented Dec 9, 2022

Let's not forget about Python too! Please!

@uchagani
Copy link
Contributor Author

uchagani commented Jan 8, 2023

Here is how to do it with JUnit 5:

import org.junit.jupiter.api.Test;

import static com.microsoft.playwright.assertions.PlaywrightAssertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertAll;

public class TestSoftAssertions extends TestBase {
  @Test
  void testSoftAssertions() {
    page.setContent("<div id=node>Text content</div>");
    Locator locator = page.locator("text=content");

    assertAll(
      () -> assertThat(locator).hasAttribute("id", "force fail"),
      () -> assertThat(locator).hasText("force fail")
    );
  }
}

This will work with Playwright assertions which is awesome.

@NikkTod
Copy link

NikkTod commented Jan 18, 2023

@uchagani do you have a solution for TestNG :)

@uchagani
Copy link
Contributor Author

I don't unfortunately. Personally I've moved all of my codebases to junit 5 because it's so much better than testng in terms of extensibility and functionality.

@SouhaB
Copy link

SouhaB commented May 15, 2023

+1, it's kind of blocking issue.

@kvnstp
Copy link

kvnstp commented May 20, 2023

+1

1 similar comment
@annuhemnani
Copy link

+1

@uchagani
Copy link
Contributor Author

I've created a new package that brings soft assertions to Playwright-Java: https://github.com/uchagani/playwright-java-soft-assertions

@yury-s @dgozman I have an open PR for this implementation to be merged into this repo if you guys like the approach. The full implementation is done and i can copy it over quickly but i'll need to update the tests to be more like how playwright does them which should only take a day or so. Please let me know because i believe this belongs in this repo instead of a separate package.

@yury-s
Copy link
Member

yury-s commented Sep 6, 2023

@uchagani

We discussed the feature and its current implementation in the API review meeting today and several concerns were raised. We are going to revert the implementation for the upcoming release at least. Main concerns:

  1. Soft assertions already work today with JUnit assertAll as you mentioned above:
assertAll("Page assertions",
        () -> assertThat(page).hasTitle("Playwright"),
        () -> assertThat(page).hasURL("foo")
  1. The implementation does not bring use close to integration with TestNG SoftAssert. At first glance, it seems that the assertions will need to implement IAssert somehow. With current implementation people who want to use builtin soft asserts and playwright's will end up having two instances of SoftAssert and SoftAssertions and two calls of assertAll.
  2. If we consider these asserts an integration with AssertJ, that's okay, though it would be a bit different proposition. In that case we have to ensure that Playwright assertions integrate nicely with the builtin soft assertions and allow further extension. As far as I can tell the following code could do that for Page:
// PwPageSoftAssertions.java
package org.example.test;

import com.microsoft.playwright.Page;
import org.assertj.core.api.SoftAssertions;

public class PwPageSoftAssertions extends SoftAssertions {
    public PwPageAssert assertThat(Page actual) {
        return proxy(PwPageAssert.class, Page.class, actual);
    }
}

// PwPageAssert.java
package org.example.test;

import com.microsoft.playwright.Page;
import com.microsoft.playwright.assertions.PageAssertions;
import org.assertj.core.api.AbstractAssert;

import java.util.regex.Pattern;

import static com.microsoft.playwright.assertions.PlaywrightAssertions.assertThat;

public class PwPageAssert  extends AbstractAssert<PwPageAssert, Page> implements PageAssertions {
    private final PageAssertions pageAssertions;

    public PwPageAssert(Page page) {
        super(page, PwPageAssert.class);
        pageAssertions = assertThat(page);
    }

    @Override
    public void hasTitle(String s, HasTitleOptions hasTitleOptions) {
        pageAssertions.hasTitle(s, hasTitleOptions);
    }

    @Override
    public void hasTitle(Pattern pattern, HasTitleOptions hasTitleOptions) {
        pageAssertions.hasTitle(pattern, hasTitleOptions);
    }

    @Override
    public void hasURL(String s, HasURLOptions hasURLOptions) {
        pageAssertions.hasURL(s, hasURLOptions);
    }

    @Override
    public void hasURL(Pattern pattern, HasURLOptions hasURLOptions) {
        pageAssertions.hasURL(pattern, hasURLOptions);
    }

    @Override
    public PageAssertions not() {
        return pageAssertions.not();
    }
}

//  Test method
  @Test
  public void testAssertjPageSoftAssertions() {
    Playwright playwright = Playwright.create();
    Browser browser = playwright.chromium().launch();
    BrowserContext context = browser.newContext();
    Page page = context.newPage();
    page.navigate("https://playwright.dev");

    PwPageSoftAssertions softly = new PwPageSoftAssertions();
    softly.assertThat(page).hasTitle("Playwright");
    softly.assertThat(page).hasURL("foo");
    softly.assertThat(2).isEqualTo(1);
    softly.assertAll();
  }

Current implementation of SoftAssertions does not help in this, moreover it encourages users to create a separate instance instance of SoftAssertions.

We decided to hold off this feature until these concers are addressed.

@yury-s yury-s reopened this Sep 6, 2023
yury-s added a commit to microsoft/playwright that referenced this issue Sep 6, 2023
As explained in
microsoft/playwright-java#819 (comment)
we are not ready to ship this in its current form. Reverting for now.

This reverts commit 475c96d.
@uchagani
Copy link
Contributor Author

uchagani commented Sep 7, 2023

The motivation behind #1361 was to enable calling playwright-java's auto-waiting assertions in a soft way, without tying it to any other library, similar to playwright-java's assertions themselves. They are not assertj assertions or testng assertions.

Soft assertions already work today with JUnit assertAll

While JUnit's soft assertions work, it is awkward. You would basically have to wrap the whole test in an assertAll to be able to do multiple actions, then assertions, then more actions, etc.

The implementation does not bring use close to integration with TestNG SoftAssert.

This was never the intention of #1361 because then we are assuming that the user is using TestNG. We can certainly provide better integration with TestNG similar to what we're doing with JUnit in #1371 but I think these are two different things: soft assertions for playwright vs soft assertions for playwright for TestNG.

If we consider these asserts an integration with AssertJ, that's okay, though it would be a bit different proposition

I think this is the same issue as tying soft assertions to another library. Since playwright-java's assertions are not tied to AssertJ I don't see why the calling these same assertions in a soft way has to be. If we tie it to AssertJ now we are limiting users to AssertJ and not other matchers that they might be using just because they want to call them softly.

it encourages users to create a separate instance instance of SoftAssertions.

This is correct but i'm open to other suggestions on how to handle this without tying it to a particular test runner (JUnit/TestNG) or a particular matcher (AssertJ/Hamcrest/).

playwright-java's assertions are self contained so that's what #1361 does. We can provide support for AssertJ, Hamcrest, JUnit, and TestNG separately just for soft assertions but that seems like an odd thing to do when playwright-java's assertions don't care which test runner or assertion library one is using.

@yury-s
Copy link
Member

yury-s commented Sep 11, 2023

The motivation behind #1361 was to enable calling playwright-java's auto-waiting assertions in a soft way, without tying it to any other library, similar to playwright-java's assertions themselves. They are not assertj assertions or testng assertions.

I understand the objective was to provide soft assertions in a manner that does not tie it to any other libraries. There is a difference, though, between playwright web assertions and soft assertions on top of them. Playwright assertions introduced features not available before (auto-waiting). On the other hand, while adding soft assertions is definitely a convenience, similar functionalities could be implemented within existing testing frameworks using playwright assertions. Different testing frameworks take different approaches to soft assertions and if we'd like to be idiomatic, we need to play by their rules and provide a more seamless integrations. This is one of the reasons we were hesitant bringing this feature to java right away. We should have spotted this earlier.

Since there is no clear-cut solution that works for all frameworks, we usually hold-off shipping any suboptimal implementations and this what I suggest we do here. We can finish JUnit integration and then can see how this can fit into the new strucutre. Does it make sense?

Soft assertions already work today with JUnit assertAll

While JUnit's soft assertions work, it is awkward. You would basically have to wrap the whole test in an assertAll to be able to do multiple actions, then assertions, then more actions, etc.

Even though it may be cumbersome, this is how JUnit recommends to do soft assertions and changing their framework is not our goal. From our experience, it's usually sufficient to have a block of multiple soft asserts followed by actions, followed by another block of soft asserts. One rarely needs to check soft assertions scattered across distant parts of the test body. In those scenarions assertAll will work just fine.

The implementation does not bring us close to integration with TestNG SoftAssert.

This was never the intention of #1361 because then we are assuming that the user is using TestNG. We can certainly provide better integration with TestNG similar to what we're doing with JUnit in #1371 but I think these are two different things: soft assertions for playwright vs soft assertions for playwright for TestNG.

That was not a goal indeed. During the initial code review I was under impression that there is no built-in support in the current frameworks and everyone does it their own way and having SoftAsserts/assertAll was one of the best practices. On a closer look, it turned out that the client will likely have to reimplement SoftAsserts if they want good integration with existing builtin soft asserts. I should have spotted this problem earlier. This is our regular practice in Playwright to no ship a feature (especially in language ports) if there is controversy.

If we consider these asserts an integration with AssertJ, that's okay, though it would be a bit different proposition

I think this is the same issue as tying soft assertions to another library. Since playwright-java's assertions are not tied to AssertJ I don't see why the calling these same assertions in a soft way has to be. If we tie it to AssertJ now we are limiting users to AssertJ and not other matchers that they might be using just because they want to call them softly.

it encourages users to create a separate instance instance of SoftAssertions.

This is correct but i'm open to other suggestions on how to handle this without tying it to a particular test runner (JUnit/TestNG) or a particular matcher (AssertJ/Hamcrest/).

I'd focus on JUnit integration and then get back to this. I believe it'd be better if we provide a solid integration with one of the testing frameworks, e.g. AssertJ, than if we end up with an implementation that doesn't integrate well with any framework.

playwright-java's assertions are self contained so that's what #1361 does. We can provide support for AssertJ, Hamcrest, JUnit, and TestNG separately just for soft assertions but that seems like an odd thing to do when playwright-java's assertions don't care which test runner or assertion library one is using.

I tried to explain above the difference from our perspective, basically soft assertions can be implemented using builtin mechanisms in each of these frameworks and playwright assertions APIs.

Germandrummer92 pushed a commit to OctoMind-dev/playwright that referenced this issue Oct 27, 2023
…icrosoft#26917)

As explained in
microsoft/playwright-java#819 (comment)
we are not ready to ship this in its current form. Reverting for now.

This reverts commit 475c96d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants