OrchardCore icon indicating copy to clipboard operation
OrchardCore copied to clipboard

UI testing with C# tests and Selenium with the Lombiq UI Testing Toolbox

Open Piedone opened this issue 3 years ago • 12 comments

This is to gather feedback on to whether continue with the initial extended PoC I've done under https://github.com/OrchardCMS/OrchardCore/pull/11194. So, please share your thoughts, and check out the code!

Is your feature request related to a problem? Please describe.

We need some automated tests that also test the UI of the app, so we can make sure that not just the individual units work well, but also the whole app, when interacted with from a browser (or with its web APIs). For this, we currently have OrchardCore.Tests.Functional that uses Cypress to run tests.

These tests are really useful, but there are aspects that I'd change/improve:

  • They're written in JS. My personal preference (admittedly, a preference) is C#.
  • They don't test much functionality: We have tests that set up the app with the built-in recipes, check that the home page has a required element, and if logging in to the admin works. While testing every single piece of functionality in Orchard like this is infeasible (it'd be a lot of work to write the tests, and a lot of time to run them), there are some further basic functionality that we could check to have some rudimentary smoke testing.
  • They don't verify that the app is without errors otherwise: The tests will pass if the explicit checks are OK, but there can be browser errors in the console (like https://github.com/OrchardCMS/OrchardCore/issues/12688), server-side exceptions, HTML validation issues (like https://github.com/OrchardCMS/OrchardCore/pull/11425), web content accessibility issues (like https://github.com/OrchardCMS/OrchardCore/issues/11242).
  • They don't run under Windows (the basic tests perhaps can be configured like that simply, but the tests for DBs apart from SQLite need significant changes.
  • They only test local storage (not Azure Blob or AWS S3).

Describe the solution you'd like

The proposal here is to use the Lombiq UI Testing Toolbox instead of Cypress, write xUnit tests, and add more of them. To see how this would look like, I added a PoC under the https://github.com/OrchardCMS/OrchardCore/pull/11194 PR. Please check out the code and the demo screensharing but this currently does the following:

  • Runs the setup with all built-in recipes, with all supported DBs, as well as a simple MVC sample, and checks basics. - This was already the case with the current tests, but the tests are now written in C# with xUnit.
  • Tests more functionality for each recipe (depending on the recipe): Apart from the setup and admin log, it tests the following:
    • Basic setup screen validation.
    • Registration, including negative tests with invalid registration form, already registered user
    • Negative login tests, i.e. you can't log in if you shouldn't.
    • Basic content item operations.
    • Turning features on and off.
    • Logout.
    • Soon will test basic Media operations too: https://github.com/Lombiq/UI-Testing-Toolbox/issues/222.
  • Checks are done automatically for every page to make sure there are no errors in the browser console, no errors in the Orchard log, there are no HTML validation errors, and no accessibility issues on the frontend (admin is excluded in the configuration but can be added once we make the admin accessible).
  • All test and DB combinations run both under Ubuntu and Windows.
  • There's a test to test the basics of using Media with Azure Blob Storage.
  • There's a monkey test for the admin area that tries to break the app with random interactions. This is to surface issues that we otherwise wouldn't notice. See a demo here.
  • Since the test and the app runs in the same .NET project, you can step into the web app's code during debugging, or see exceptions right away. See this demo.

Note that the PR currently runs all the tests, with all feature tests indiscriminately, so testing alone is 6-7 minutes on GitHub Actions. This is something we can fine-tune:

  • Only running a barebone of tests, not all of them or not with all feature tests.
  • Only run under Ubuntu (which is faster than Windows) for each PR, and everything only for merge commits in main (that nobody needs to wait for).
  • Make use of setup snapshots: Currently, all tests run the Orchard setup at start. Since some tests run the setup with the same recipe, the resulting DB can be cached. This is supported by the UI Testing Toolbox, but only for SQLite and SQL Server, so due to Postgres and MySQL it's currently not utilized at all, but can be just when testing with SQLite or SQL Server.

However, if we want to get most of the benefits even for PR builds, I expect it to take more time than the 1-2 minutes of Cypress tests currently.

Potential future improvements:

  • This can also pave the way to make the admin area conform to accessibility guidelines too.
  • While the linked PR doesn't yet utilize it, we could also add visual verification testing to some of the key pages to make sure that we don't mess up their styling accidentally.
  • Apart from running tests with Chrome, we can run them with Edge and Firefox too.

Describe alternatives you've considered

Extending the Cypress tests to cover the above-mentioned areas to improve. I personally don't want to dive into that though.

Piedone avatar Nov 17 '22 16:11 Piedone

NB: The ASP.NET repository is getting rid of all selenium tests (too many issues after updates), and moving to Playwright.

sebastienros avatar Dec 08 '22 19:12 sebastienros

FYI I already did some work using Playwright few weeks ago and testing in general, hope to share it with you in upcoming weeks. Also I mention Playwright here

One more thing to know that Playwright doesu n't support xUnit at the time I playing with it, then logged an issue in their repo, but it's closed :(

hishamco avatar Dec 08 '22 20:12 hishamco

I think Playwright is the next thing we should look at. Though we need a POC for that and we need to see what is missing in Playwright because it is probably less mature than everything else out there.

Skrypt avatar Dec 14 '22 22:12 Skrypt

@sebastienros can you point to something that shows details about that move to Playwright?

Getting to the same point where we are now with these tests, but with Playwright, would be a huge amount of work, so I suggest not starting with that :D.

Piedone avatar Dec 15 '22 00:12 Piedone

@Piedone https://github.com/dotnet/aspnetcore/pull/45284

sebastienros avatar Dec 15 '22 00:12 sebastienros

Thanks!

Piedone avatar Dec 15 '22 00:12 Piedone

Last call: Please let me know if you think this is worth continuing working on. If not, I'll close the issue and PR.

Piedone avatar Jan 13 '23 13:01 Piedone

I don't want to decide. I am personally afraid of the complexity of the solution, the new dependencies (selenium, which we removed) and don't see much value in changing right now.

But I don't write functional tests, so I'll let those who write/fix them to decide. I just want to ensure this is not adding too many complexity and maintenance burden.

sebastienros avatar Jan 19 '23 18:01 sebastienros

I might create a PR for UI with Playwright which I play with it few months ago, but again there're some cool things that done by Lombiq that we can add to the new OrchardCore.Tests.UI

hishamco avatar Jan 19 '23 19:01 hishamco

Well, we currently also have dependencies for functional tests, just as doing them in Playwright would add them, and any approach adds complexity over not having such tests. Compared to what we have now, the PoC I've created adds complexity, because it's testing a lot more things.

Piedone avatar Jan 19 '23 19:01 Piedone

FYI https://github.com/dotnet/aspnetcore/pull/45682

hishamco avatar Feb 14 '23 16:02 hishamco

I think the goal should be that every module feature should have at least one UI test as well. It doesn't mean that every possible functionality should be UI tested, but every module feature should have a corresponding smoke test at least, that makes sure that the feature is not broken in some very obvious, fundamental way (that starts with making sure that enabling the feature still works, what can break too, and if it has recipes, executing them also works).

Piedone avatar Jun 19 '24 15:06 Piedone

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

github-actions[bot] avatar Nov 12 '24 23:11 github-actions[bot]