Importance and requirements of testing in Chromium

[Elliott Sprehn]

Hey Folks!

A while ago I was writing a patch changing some painting behavior when loading stylesheets. The change was very exciting with a big impact, but my code reviewer pdr@ was concerned at my lack of tests. I protested saying that testing the racy behavior of stylesheets loading was very hard with LayoutTests, that historically we had landed some changes like this without tests, and that my change was totally awesome. pdr@ rightly challenged me again saying tests were critical to the project. So I stepped back and took a few weeks to write the SimTest testing framework. It did actually catch some bugs in my patch, and we now have great test coverage of loading stylesheets. We've also since used this testing system to test many more features. pdr@ was totally right, my patch needed tests. :)

There's a couple lessons here:

Automated testing is a requirement

Automated testing is critical to our ability to ship a high quality product on our aggressive release schedule. It's a requirement for all code that it have automated tests. If that's hard, please reach out to platform-architecture-dev@ for help. If it still can't be done we can figure out how to check-in manual tests, but that's a last resort.

Any change to a web API should either change tests directly or link to the tests in the commit description. If your feature has tests outside the repro, like webgl, then you should make sure to follow up on your patch with a link to the tests in the code review, or ideally do this first and link to the external CL for the tests in the commit description of the chromium change.

Note that you don't have to focus on interop testing from day one either: sometimes your spec has lots of churn or you're not quite sure how to mock the OS APIs properly. Focus on having good internal test coverage in these cases first. Once your feature is stable you can reach out to 

platform-predictability@ for help writing interop tests.

Reviewers should check for tests

Reviewers should question all patches that don't add or modify tests. It's never bad etiquette to question missing tests on a patch. In fact, it's expected of you as an OWNER. Don't take this personally as an author, and don't stress about how it delays your feature. The continued quality of your feature depends on automated testing. Working on tests is never wasted time.

This is especially true for web platform tests where changes can cause burn in on the web or break sites. If the change you're reviewing is observable (ex. JS API), make sure there's tests.

Testing hard things pays off

If testing your feature is hard, it's likely that other folks have similar features where testing was hard. If you put in the effort to test your feature they can reuse the work you did and improve the quality of their feature too.

Note that if your feature is hard to test end to end due to OS dependencies (ex. Android APIs) use mojo mocks in blink and unit tests on the browser side. End to end tests are great, but often very hard to land or are flaky. It's okay to start with coverage that uses mocks for either side of the IPC boundary. If this still seems hard in your design please reach out to platform-architecture-dev@ for help.

Thanks everyone for putting so much effort into testing! Your continued commitment to testing keeps chromium awesome.

- E

[Kenneth Russell]

On Mon, Dec 19, 2016 at 7:49 PM, Elliott Sprehn <esp...@chromium.org> wrote:

Hey Folks!

A while ago I was writing a patch changing some painting behavior when loading stylesheets. The change was very exciting with a big impact, but my code reviewer pdr@ was concerned at my lack of tests. I protested saying that testing the racy behavior of stylesheets loading was very hard with LayoutTests, that historically we had landed some changes like this without tests, and that my change was totally awesome. pdr@ rightly challenged me again saying tests were critical to the project. So I stepped back and took a few weeks to write the SimTest testing framework. It did actually catch some bugs in my patch, and we now have great test coverage of loading stylesheets. We've also since used this testing system to test many more features. pdr@ was totally right, my patch needed tests. :)

Awesome work Elliott! Thanks for writing this up and sharing.

 

There's a couple lessons here:

Automated testing is a requirement

Automated testing is critical to our ability to ship a high quality product on our aggressive release schedule. It's a requirement for all code that it have automated tests. If that's hard, please reach out to platform-architecture-dev@ for help. If it still can't be done we can figure out how to check-in manual tests, but that's a last resort.

Couldn't agree more with this. The Chromium project is massive, and the only way to feasibly ensure its quality is to test it in automated fashion. I've been humbled as a software engineer with the project's scale, and have come to realize that automated testing is the primary piece of infrastructure that has allowed it to grow.

Just as crucially, these automated tests need to be reliable. Test flakiness is a significant problem. It leads to retries and slowdowns on the commit queue, and is a top source of toil for developers and sheriffs. All developers on the project need to take responsibility for the tests in their area and make sure they're running reliably. Eliminating test flakes should be considered a top priority.

-Ken

[Yoav Weiss]

Thank you Elliott for writing this down!

I'd like to ask for some guidance regarding the interaction between CLs and WPT tests.

I'm currently reviewing a CL which tests are already written as WPT, waiting for review on that end.

What's the best course of action in such a situation?

1) Wait for WPT to land and import them before landing related CLs (which don't include their own tests)?

2) Try the new process added in crbug.com/657117 which enables us to land tests directly in LayoutTests/imported/wpt/ and have them exported automatically?

3) Land the changes and hope the WPT tests pass review there shortly?

4) Other???

As we (rightfully) encourage writing more tests as WPT, I feel like this kind of situation might be more frequent.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Marijn Kruisselbrink]

On Thu, Dec 22, 2016 at 1:51 AM, Yoav Weiss <yo...@yoav.ws> wrote:

Thank you Elliott for writing this down!

I'd like to ask for some guidance regarding the interaction between CLs and WPT tests.

I'm currently reviewing a CL which tests are already written as WPT, waiting for review on that end.

What's the best course of action in such a situation?

1) Wait for WPT to land and import them before landing related CLs (which don't include their own tests)?

2) Try the new process added in crbug.com/657117 which enables us to land tests directly in LayoutTests/imported/wpt/ and have them exported automatically?

3) Land the changes and hope the WPT tests pass review there shortly?

nit: Not sure what the "hope" here means. Whoever would be reviewing the tests in case 2) should be just as capable of reviewing the tests downstream directly, right? So there really shouldn't be any hoping and tests would at least land at WPT at the same time or earlier than the actual code would land. Of course that still leaves the question if code should land before the tests have been imported, but there really shouldn't be any reason for "hope" in this process.

4) Other???

As we (rightfully) encourage writing more tests as WPT, I feel like this kind of situation might be more frequent.

On Tue, Dec 20, 2016 at 4:50 AM Elliott Sprehn <esp...@chromium.org> wrote:

Hey Folks!

A while ago I was writing a patch changing some painting behavior when loading stylesheets. The change was very exciting with a big impact, but my code reviewer pdr@ was concerned at my lack of tests. I protested saying that testing the racy behavior of stylesheets loading was very hard with LayoutTests, that historically we had landed some changes like this without tests, and that my change was totally awesome. pdr@ rightly challenged me again saying tests were critical to the project. So I stepped back and took a few weeks to write the SimTest testing framework. It did actually catch some bugs in my patch, and we now have great test coverage of loading stylesheets. We've also since used this testing system to test many more features. pdr@ was totally right, my patch needed tests. :)

There's a couple lessons here:

Automated testing is a requirement

Automated testing is critical to our ability to ship a high quality product on our aggressive release schedule. It's a requirement for all code that it have automated tests. If that's hard, please reach out to platform-architecture-dev@ for help. If it still can't be done we can figure out how to check-in manual tests, but that's a last resort.

Any change to a web API should either change tests directly or link to the tests in the commit description. If your feature has tests outside the repro, like webgl, then you should make sure to follow up on your patch with a link to the tests in the code review, or ideally do this first and link to the external CL for the tests in the commit description of the chromium change.

Note that you don't have to focus on interop testing from day one either: sometimes your spec has lots of churn or you're not quite sure how to mock the OS APIs properly. Focus on having good internal test coverage in these cases first. Once your feature is stable you can reach out to 

platform-predictability@ for help writing interop tests.

Reviewers should check for tests

Reviewers should question all patches that don't add or modify tests. It's never bad etiquette to question missing tests on a patch. In fact, it's expected of you as an OWNER. Don't take this personally as an author, and don't stress about how it delays your feature. The continued quality of your feature depends on automated testing. Working on tests is never wasted time.

This is especially true for web platform tests where changes can cause burn in on the web or break sites. If the change you're reviewing is observable (ex. JS API), make sure there's tests.

Testing hard things pays off

If testing your feature is hard, it's likely that other folks have similar features where testing was hard. If you put in the effort to test your feature they can reuse the work you did and improve the quality of their feature too.

Note that if your feature is hard to test end to end due to OS dependencies (ex. Android APIs) use mojo mocks in blink and unit tests on the browser side. End to end tests are great, but often very hard to land or are flaky. It's okay to start with coverage that uses mocks for either side of the IPC boundary. If this still seems hard in your design please reach out to platform-architecture-dev@ for help.

Thanks everyone for putting so much effort into testing! Your continued commitment to testing keeps chromium awesome.

- E

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

[Rick Byers]

/cc platform-predictability where we've been discussing WPT process

On Thu, Dec 22, 2016 at 12:17 PM, Marijn Kruisselbrink <m...@chromium.org> wrote:

On Thu, Dec 22, 2016 at 1:51 AM, Yoav Weiss <yo...@yoav.ws> wrote:

Thank you Elliott for writing this down!

I'd like to ask for some guidance regarding the interaction between CLs and WPT tests.

I'm currently reviewing a CL which tests are already written as WPT, waiting for review on that end.

What's the best course of action in such a situation?

3) Land the changes and hope the WPT tests pass review there shortly?

nit: Not sure what the "hope" here means. Whoever would be reviewing the tests in case 2) should be just as capable of reviewing the tests downstream directly, right? So there really shouldn't be any hoping and tests would at least land at WPT at the same time or earlier than the actual code would land. Of course that still leaves the question if code should land before the tests have been imported, but there really shouldn't be any reason for "hope" in this process.

Right, if you like you can ask the person reviewing your chromium CL (or any other chromium reviewer with the necessary context) to also review the WPT tests on GitHub.  Once they add an LGTM than feel free to ping me, foolip, tkent or anyone else with WPT push access to merge.

 

1) Wait for WPT to land and import them before landing related CLs (which don't include their own tests)?

Automatic imports are now happening nearly daily (and if not you can ping qyearsley to get an import done).  So given the above, there shouldn't be more than a day of additional waiting or so for this.  So when things are working properly this choice shouldn't be that bad.  But:

2) Try the new process added in crbug.com/657117 which enables us to land tests directly in LayoutTests/imported/wpt/ and have them exported automatically?

 

This is what we think the right long-term answer is for most cases (cases where you're not explicitly looking for review from other vendors - just landing tests along with your blink patches, etc.).  Jeff/Philip tell me we're looking to start dogfooding this more.  It's not quite ready for prime-time, but we have been landing some WPT patches this way.  So if you're willing to be a guinea pig and give jeffcarp@ feedback, then yes please go ahead and start trying this out.

4) Other???

As we (rightfully) encourage writing more tests as WPT, I feel like this kind of situation might be more frequent.

Absolutely.  Predictability Q4 KR for this is "Blink developers are happy working with web-platform-tests instead of LayoutTests" and IMHO we've made good progress but aren't done yet.  Philip will be giving a talk about WPT workflow at BlinkOn, and we'll probably send out announcements and links to docs around that time.