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

Run Selenium tests on Playwright via an adapter [experiment] #45284

Closed
wants to merge 6 commits into from

Conversation

SteveSandersonMS
Copy link
Member

This is just an experiment to see what the team thinks.

Premise

Even though the Selenium-based E2E tests have been very reliable for more than a year now, we still incur costs from them:

  • Keeping the Selenium dependency itself up-to-date is expensive: we have to keep doing manual processes, and the updates don't always just work. For example, we're really struggling to update it in this PR.
  • Longer term, it's obvious that Selenium is not particularly modern and carries lots of legacy baggage. If we were starting from scratch, we'd surely choose Playwright instead.

But we can't just casually rewrite all these tests in Playwright. For one thing, there are a lot of them and this would be many weeks (maybe months) of effort, but also there's a huge amount of subtle thought behind they way they are constructed. For example they are testing things like which UI updates should occur synchronously vs asynchronously, and when certain updates should not occur. A casual rewrite is likely to end up producing tests that pass but don't capture all the behaviors we captured before, and we lose vital coverage.

An adapter

If the problem isn't the existing tests themselves, but rather is the having to depend on Selenium's underlying technology stack (and keep updating it, etc.), then what about if we had an adapter so the existing tests could run on Playwright?

This PR is a quick experiment to see how practical that might be. I've set up a new test project and pulled in an existing test fixture (StandaloneAppTest.cs) via shared-source. To make it build, I've made a set of APIs equivalent to the shape of Selenium's APIs, and their implementation uses Playwright. This does work and the existing test code from that fixture passes. I did have to skip the fixture's Dispose method but otherwise the test code is unchanged.

If we were to do this fully, the test coverage should be unchanged, and the payoff would be:

  • We could remove the actual Selenium dependency and all the processes around updating it
  • If we wanted, the existing tests could be run against other browsers than Chrome (maybe even against a WebView too)
  • Since all the real Playwright APIs are also available, any new or updated tests can simply be written using Playwright directly without needing any further infrastructure

There are certainly drawbacks though:

  • The test code is then strange. A casual reader will think it uses Selenium (it even imports namespaces like OpenQA.Selenium) but it actually doesn't.
  • The adapter has to do loads of sync-over-async (e.g., evaluating someTask.Result or calling task.Wait()). Obviously that's horrible, but what else can an adapter like this do?
    • Longer term we might hope to migrate away from Selenium's API patterns but at least we can do so incrementally.
  • We might make mistakes in how we map Selenium semantics to Playwright's APIs. We would have to be careful.
  • There's still a lot of work involved in making this into a real, complete adapter sufficient to run all the existing E2E tests.
  • Playwright isn't exactly a utopia of simplicity either. Trying to make it download the right versions of the browsers it wanted took me at least an hour this morning. This is probably because of the massive stack of C#/Xunit/MSBuild customizations and abstractions we've already built around it (honestly I don't know what most of Microsoft.AspNetCore.BrowserTesting does, or what we've done to make things run in Helix).

Let's come up with a plan

I'm OK with discarding this approach if people aren't into it. If we were going to do something like this, we would need to make a plan for finishing the work, i.e., switching over each of the test fixture classes (possibly incrementally) and expanding the adapter API surface as we go to make it all still build and pass.

@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner November 25, 2022 16:24

<ItemGroup>
<Compile Include="..\E2ETest\Infrastructure\ServerFixtures\*.cs" LinkBase="Infrastructure\ServerFixtures" />
<Compile Include="..\E2ETest\Tests\StandaloneAppTest.cs" LinkBase="Tests" />
Copy link
Member Author

Choose a reason for hiding this comment

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

Here you can see we're actually using the real StandaloneAppTest.cs test fixture, and it now does run and succeed on Playwright.

public void Dispose()
{
// Make the tests run faster by navigating back to the home page when we are done
// If we don't, then the next test will reload the whole page before it starts
Browser.Exists(By.LinkText("Home")).Click();
}
#endif
Copy link
Member Author

Choose a reason for hiding this comment

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

It's probably possible to make this work without being #if-ed out. I only disabled it because of an execution ordering issue (it was tearing down the Playwright browser first, then calling this, which threw because it could no longer talk to the browser).

@captainsafia
Copy link
Member

Playwright isn't exactly a utopia of simplicity either. Trying to make it download the right versions of the browsers it wanted took me at least an hour this morning. This is probably because of the massive stack of C#/Xunit/MSBuild customizations and abstractions we've already built around it (honestly I don't know what most of Microsoft.AspNetCore.BrowserTesting does, or what we've done to make things run in Helix).

Can you share more on this? Would you say most of the difficulty came from complexity in our repo or with Playwright itself?

The test code is then strange. A casual reader will think it uses Selenium (it even imports namespaces like OpenQA.Selenium) but it actually doesn't.

Agree with this! Although I think we can alleviate this a little bit by putting all the tests that use the adapter in a separate directory or otherwise labeling them in a way that says "this isn't really selenium, it just looks like it".

@SteveSandersonMS
Copy link
Member Author

Can you share more on this? Would you say most of the difficulty came from complexity in our repo or with Playwright itself?

It's been a few months so I don't have a fresh memory of it, but some of the issues were:

  • ... from our repo: trying to reverse-engineer exactly how our custom system causes settings for Playwright get into it at runtime, and which settings would actually get used or ignored. It's an antipattern that commonly that arises in large codebases like ours, rather similar to the inner platform effect. In this case, we've customized a lot how the build mechanism works and how Playwright get used, so the standard defaults from other projects no longer work for us, forcing us to customize even more things - a vicious cycle.
  • ... from Playwright itself: despite an hour or so of going through docs repeatedly and running experiments, it was very tough to make it acquire and use the right version of the Chromium driver. The disconnect may be something to do with the NPM acquisition path being treated as more first-class than the .NET one. Most likely others wouldn't face this issue as they would be in a more default environment, so you could argue this is just another manifestation of the first issue.

@mkArtakMSFT mkArtakMSFT added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jan 18, 2023
@ghost
Copy link

ghost commented Jan 26, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please remove the pending ci rerun label to kick off a new CI run.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 26, 2023
@SteveSandersonMS SteveSandersonMS marked this pull request as draft August 23, 2023 12:32
@SteveSandersonMS
Copy link
Member Author

I'm closing this now since it's been here for months and we don't have any immediate plan to take action on it. It could be reopened if we change our minds. @mkArtakMSFT

@SteveSandersonMS SteveSandersonMS deleted the stevesa/e2e-test-adapter branch November 16, 2023 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants