serlo / cloudflare-worker

Cloudflare worker which works as a proxy for https://serlo.org/
Apache License 2.0
9 stars 1 forks source link

☂️ Umbrella Issue: Test improvements (RFC) #76

Closed kulla closed 3 years ago

kulla commented 3 years ago

Abstract

I have learned a lot about how to write good tests in the last months (mostly by https://kentcdodds.com/blog/ who's philosophy inspired https://testing-library.com/docs/intro). The main takeaway when it comes to mocking services is:

The mocked services shall mimic the behavior of the real services.

An example: Instead of intercepting an outgoing request to see whether it has the right authorization header we write the service in a way that when it receives a request without a proper authorization header it answers with "403 Forbidden". See https://mswjs.io/docs/recipes/request-assertions

Another similar advice is the following:

Tests should never test implementation details. When we write a test for a piece of software we should consider who the consumer of this software unit will be (a developer using the module like he would use a library or a user of serlo doing requests with his browser). How can she / he observe the feature which shall be tested? What steps does she / he need to do? Then implement the test by putting those steps into code. Looking into the module (e.g. testing implementation details) should be avoided since the consumer won't be able to do it either.

The cloudflare worker is our proxy which implements some services (e.g. rendering the imprint / terms) or does other jobs (like redirecting). A user of serlo will be the main consumer of the cf worker. Thus we need to ask ourselfs: How can a user of serlo notice a particular feature (notice that he / she has no control nor can he / she look into databases, services like api.serlo.org nor he / she can intercept requests in our network)?

Example: We want to test the api module which adds authorization header to calls to api.serlo.org. To test this feature in the real world (for example in our staging environment) we would simply make an api call in this environment. When it succeeds we know that everything is ok since api.serlo.org checks for the authorization header and would deny the request in case it is not present or wrong.

When we follow those philosophies we can achieve the following:

We can write tests which can be run locally by mocking not present services and against real environments like staging or dev. Thus we can reuse the tests for testing the cloudflare worker at staging and dev as well (which in a future we might want to do in a cron job to monitor those environments).

When we archive this we have some advantages:

Our tests in the cloudflare worker do not fulfill those requirements yet. However I want to change it and I want to test this philosophy here to see whether the efforts are worth it.

Propsed changes

I want to rewrite the tests in a way that they can also be used to be executed against our test environments staging / dev. The necessary changes are:

However not all tests can be written in this way. There are some problems to solve (see next section). We might also want to test some utility function (like for example getCookieValue()) to get confidence that it works as intended. Here we can treat it like an external library and test it this way. To make the distinction between tests for the whole cf worker and some internal modules we can have two dictionaries:

Problems to solve / questions

Ideas for the future

In case we have tests which change an environment (like adding an article instead for example reading the imprint) we can flag those mutating test cases. So we can run the tests also against production but only those who do not change it. These tests can be used by a cron job to monitor the production environment and to detect bugs there fast -> so hopefully no end user will need to report a bug which we should have already covered by our tests since we already know it presence in advance we can thus fix it.

Comments appreciated since I would start to rewrite our tests in this direction. However achieving frontend 100% is more important currently :smile: Ping @inyono since this is important to know for my current PRs.

Concrete issues:

kulla commented 3 years ago

I am closing this issue, since the most important changes are in master. It is not fully implemented. However there are follow up issues tracking the missing features.