Let's make 2.1 about quality!

[Piedone]

We're nearing the 1.9 2.0 release. For 1.10 2.1, before we can think of what we want for 3.0, I think we should focus on quality, and to maintain that quality, automated Quality Assurance (QA). The latter is especially important because we have very little automation for QA currently, and thus suffer frequent regressions, as well as the necessity to test PRs extensively by hand (during my reviews, I frequently have to point out fundamental things broken in PRs, what should all be caught by tests instead).

This is what I propose after 1.9 is released:

• Only accept new PRs that are for the below areas, for bug fixes, basic maintenance (like dependency updates) and non-code improvements (e.g. docs, GitHub Actions workflows). So, no new PRs are merged for anything else, like features or refactoring (PRs opened previously may be merged).
• Introduce static code analysis for C#, see Introduce static code analysis #7950 (and these PRs touch on the subject too: Add CSharpier config #15287, Add Prettier configurations #15286, Added dotnet format #12235). We have some code analyzers enabled currently, but extensive further configuration is needed.
• Rethink how we build JS/CSS, and introduce JS/CSS linting: Rethink our JS/CSS build story (Gulp) #15740.
• Add UI tests (UI testing with C# tests and Selenium with the Lombiq UI Testing Toolbox #12834 and Create a functional test for content preview #15208, also see Consider testing RTL when there's a UI change #15535) and security scanning (Security scanning with ZAP #14840).
• Our unit test suite has holes in it too (see e.g.: Indexing Providers: Unit Tests #15601, Add unit tests for Script and Style tag helpers #15504, Unit Test: GraphQL dynamic field on a content part #8670, Pager: unit or functional tests #7743). So, we should add unit tests as well.

Once we're done with these, we'll be in an excellent position to work on in-depth changes and accept a lot more community contributions. #15029 already caused more contributions, and I expect #15034 to have a similar effect too.

These are all practices that we at Lombiq use with all of our projects, including the large Lombiq's Open-Source Orchard Core Extensions solutio, all customer and in-house projects, as well as Orchard Core Commerce too. It's a large initial effort for an existing codebase, but starts to pay dividends immediately by improving the application and dramatically lowering the time spent on code reviews.

I can work on static code analysis and UI testing (for that I've actually added a draft PR previously with a lot of testing improvements and new tests but there was not enough interest at that time: #11194), if two people volunteer as my buddies who I'll discuss ideas with, and who'll quickly review and merge my PRs.

What do you all think? I'd especially ask you @agriffard @BenedekFarkas @hishamco @hyzx86 @MikeAlhayek @sarahelsaig @sebastienros @Skrypt for some feedback.

[hishamco]

I totally agree with you Zoltan, we need a lot of testing to improve the quality, I can take the initiative on testing why I like this specifically also my master's degree in Software Testing :)

In terms of testing, I have a demo that I deferred for a while to have a look into Playwright. Also, Lombiq guys invest in UI Toolbox which is great as well

In the end, I can start writing units & functional testing. what I like about some projects like Fluid is contain a lot of unit testing which is something position in terms of quality

[Piedone]

Yeah, Fluid has some great tests!

With Playwright my issue is that with UI testing, 90% of what you have to do is specific to the application and it matters much less what kind of API you use to drive the browser. And thus if we were to use Playwright instead of the UI Testing Toolbox with Selemium, there would be an insane amount of wheels to reinvent (about 4 years worth of it) to make it work well with Orchard Core, and thus let us put time into writing tests instead of creating a kind of testing framework.

[hishamco]

I did E2E testing with the OC application, Playwright has a lot of flexibility, BTW I'm not against the UI Testing Toolbox, but we could do some better if we use it in way or other from within UI Testing Toolbox

[MikeAlhayek]

While I agree with most of your summary, I don't think slowing down the momentum is a good idea.

Focusing on QA is a great agenda for 1.10 (and moving forward). But we also want to welcome any contributions. What I agree with is try to avoid merging a very-risky code unless it's backed up by tests and we feel it is valuable enough to merge at that time. Also, any refactoring should be avoided unless it also adds a test cases to support it so we are sure refactoring isn't breaking and scarify quality.

Sometimes, there are immediate needs to improve existing functionality that aren't bug fixes. We can't completely say we are not willing to accept them in 1.10 as it could become problematic for the people that relays on OC heavily.

I like the initiative you are doing and appreciate the effort. Maybe you should keep focusing on automation as you have added lots of things lately to improve the project. At the same time, if someone submits a PR, I don't think we should hold it back unless it is a very risky PR that impacts core functionality.

I think another initiative for 1.10, is to reduce the existing finalized PR. Instead of having 70, maybe cut that down to like 30 or less. If there is already a PR, the work is done or almost done otherwise it should be closed.

Meanwhile, we have some critical issue that are preventing the release of 1.9 and should be a high priority for us. Anyone that is available should take time to attempt to fix them. Here is a list of them https://github.com/OrchardCMS/OrchardCore/issues?q=is%3Aopen+is%3Aissue+milestone%3A1.9

[Piedone]

My problem is, that any change potentially contains bugs and other issues. If we don't first get a solid automated QA set up, the goalpost will keep moving while we work on it, making it harder to complete it (even with small things like we have an open PR to enforce some kind of formatting with code analyzers, then merging other PRs can make you have to re-fix formatting for the solution). We have to bite the bullet once, for like 3-4 months until 1.11 or 2.0, otherwise we will have a much harder time (and there will still be a lot of options for contributions open).

While all contributors are welcome, I don't think all contributions are. In the end, we have to maintain things in the long run, and reviews are also work. Automated QA is investing in long-term maintainability so if we decide on focusing on that for 1.10, everything else for the time being should be secondary since they make achieving it harder. I'd communicate this appropriately as well, of course, so for contributors the surprise doesn't come when their PR is already submitted.

I'd be glad to have fewer open PR, or to put it otherwise, only have open PRs for things that are actively being worked on (however many), but after #15029 I don't think there's anything else overarching to be done: every PR is waiting either for the author or the previous reviewer (and both of them are notified about this). And now if a PR isn't touched for 2,5 months, it'll be closed automatically.

Yep, those post-1.8 regressions are important. These are bugs that would've mostly been caught by UI tests.

[Skrypt]

I can't agree more. We should also try to enforce unit/functional tests to come with new Pull Requests. If not provided then we at least need to have a task about it so that it can be done within a respectable delay. Meaning that it should be done before releasing the next version so that we make sure that it is tested properly.

If a release creates regressions on current websites it should also be documented and/or we should at least provide some help with possible SQL migrations when we can't have them as migrations/recipes.

But the main point here is that we should try to fix any regressions that we find in OC before we deploy any new release. Adding new features will often create more complexity and more regressions. I agree that there is already a lot to cover with what we have and that if we want to catch up we will probably need to freeze some parts of it.

We could at least accept PR that are already really well test covered. Meaning that they should provide unit tests/functional tests.

So, in order I think we should have this Playwright PR merged first. We already have Cypress and really few people use it so I'm thinking that if Playwright creates a small buzz maybe people will start creating functional tests at least.

[Piedone]

It's not Selenium vs any other library but having single lines of UI test initialization and everything related to OC a method call available vs reinventing a framework and not being able to just focus on writing the tests themselves. See the samples: https://github.com/Lombiq/UI-Testing-Toolbox/tree/dev/Lombiq.Tests.UI.Samples#training-sections You can't do this without putting months of work into the basics first.

[Skrypt]

True, it takes time to invest into a specific tool but I think it is still relevant to ask ourselves if we want to use Selenium, Cypress or Playwright because in the end this is what we will use moving forward. Tool adoption is a big factor here because we also have Cypress functional tests that are already there since the beginning ; but no one has ever added anything to it since it's been there.

Now, I understand that Lombiq has put a lot of effort into their UI testing toolbox so it is worth to considerate also using it as it is. But this will require community adoption. So, this is why I believe we should have at least some sort of survey to see what people are thinking about it simply.

This is not a scientific approach here, we won't make any comparison between these but just ask which ones people are interested into pushing time on learning the tool. Does that make sense?

[hishamco]

No Playwright PR but @hishamco said he would have something at some point.

This was an experiment that I did a month ago, I'm still waiting for a suitable time to demo it

[Piedone]

We certainly need to weigh the options, I agree. What matters most is the preference of core contributors, since it's mostly up to them to write and maintain tests, as well as to enforce the quality of new ones written by external contributors.

[hishamco]

I will prepare two parts demo today or next week, and get the feedback. I know what I'm trying to achieve is building a testing library, so we can collaborate to take advantage of what you did in the UI Toolbox plus bring a modern & mature E2E library that Microsoft supports to OC

[Piedone]

I'll mark automated QA-related issues and ones about serious bugs with the 2.1 milestone.

[hishamco]

Many things in OC are hard to test, so it would be nice if we reactor the code or build mocks to make it easy to use them during testing. Also, the important thing here is to add more unit tests

My aim for now is to start to write more unit tests for that things that are missing, we code use code coverage tools to find those

[Piedone]

Such refactoring to make testing easier (or possible) is great, but only if it goes strictly hand in hand with adding tests.

[hishamco]

Can we create an issue for adding unit tests? I find @hyzx86 for instance struggles in testing Lucene indices if I'm not wrong

[Piedone]

There are several already, what I added to 2.1. If you find anything else, please create an issue for it (for concrete features/tests, so we don't have monster issues).

[hishamco]

Let me check with code coverage I'm sure there are many APIs that don't have proper unit tests, I remember when we released 1.0 we started seriously adding unit test for many features