duckduckgo / autoconsent

Mozilla Public License 2.0
76 stars 18 forks source link

Unit tests #333

Open muodov opened 8 months ago

muodov commented 8 months ago

Some parts of the code are complex enough that they can benefit from unit testing (as opposed to e2e tests that verify rules). For example, internal logic of handling multiple CMPs, or prioritizing non-cosmetic rules. We have something resembling unit tests for rule executors, but it's hacky and not extensible.

First thing that comes to mind is using karma, but we can consider other frameworks as well.

muodov commented 1 week ago

@freiondrej I will try to answer your questions here to consolidate the general test discussion from https://github.com/duckduckgo/autoconsent/pull/468 and https://github.com/duckduckgo/autoconsent/pull/490.

First of all, I really appreciate your enthusiasm. It is very inspiring to see external contributors like you and @seia-soto willing to donate their time to help with this project. Thank you!

As I mentioned in https://github.com/duckduckgo/autoconsent/pull/468#issuecomment-2255532560 and https://github.com/duckduckgo/autoconsent/pull/468#issuecomment-2293615088, there are different types of JS tests, and even more tools that you can use to write them. It can be difficult to figure out what you actually need, as every library claims to solve all your problems at once.

I think the best way is to start from the problem first, and focus on that. I outlined my view in https://github.com/duckduckgo/autoconsent/pull/468#issuecomment-2255532560, and if I had to pick the most impactful thing to do, it would be logic tests for DOM actions and the main Autoconsent class. The latter is not covered by unit tests at all, and for the former we have very clunky tests based on Playwright. Playwright is not a convenient tool for that. There's some hacky overhead: you need to make a fake HTML page, and encode JS snippets to eval in the page context.

It's been a while since I worked with Cypress, but as I understand it's more of a competitor to Playwright Test. That is, it's focused on e2e testing, emulating user actions, and is more suited for UI development. We have lots of e2e Playwright tests right now, which verify specific sites. If you check the implementation, you'll find that the logic is similar to your Cypress PR. They do the job well, and the only problem is flakiness due to the sites themselves changing all the time. That will apply to the Cypress tests too if they test against real websites. Similar to Playwright, it would also be awkward for logic tests, as testing individual functions and mocking is tricky. Finally, Playwright can run on Webkit (Safari) and Firefox, so I don't see value in replacing Playwright with Cypress at the moment.

The reason I suggested Web Test Runner or karma + mocha, is that they are more low-level. They essentially generate a web page that encapsulates all you your test code and the assertions. They automate browsers only to open that page, but that's it. You could also just point your own browser to a special URL, and it would work the same way. This gives two advantages for "unit" tests:

Now, to make it more concrete, how about we do this:

I imagine the test would be a JS test that roughly does the following:

  1. Create an instance of DomActions
  2. Mock some methods with sinon, so that no actual DOM is required. For example, you could mock elementSelector() or querySingleReplySelector()
  3. assert that click() (and potentially other methods) are working as expected with different arguments.

Alternatively, instead of a JS test you could explore HTML tests to test DomActions. You can probably get away with less mocks then.

Later on, if this works with DomActions, we can follow up with tests for the Autoconsent methods.

@freiondrej How does this sound? Let me know if you'd be interested to give it a try.

freiondrej commented 1 week ago

@muodov aaah I think I finally got you! 🚀 sorry, I misunderstood this comment as I thought "Webcompat issues are easy to miss, and it's a challenge for us since DDG has apps on all platforms." suggests the priority is the e2e tests, not unit ones. You were talking about unit tests being run by WTR, correct? 🙇

Also, from "Unit tests are nice to have for testing business logic. That said, I'd argue most of the codebase deals with DOM manipulation, and the algorithm itself is fairly straightforward. Mocking DOM can be annoying, and it is a significant maintenance burden.", I thought that means unit tests would be of low value to the project and that I should focus on integration / e2e with a better DX than playwright ... I got it all mixed up it seems 😂

Thank you for the concrete suggestions. I'm definitely up for it! (cannot make promises how soon I'll be able to get to that, but I will).

muodov commented 1 week ago

@freiondrej I see, sorry it was unclear. Yes, I think right now the biggest opportunity is running unit tests with (something like) WTR. They won't be "pure" unit tests because I don't think we should mock native DOM APIs, but I think it's a sweet spot between e2e tests and synthetic JS unit tests.

I'm looking forward to your PR ;)