Ghost: Server folder restructuring & test tooling & test refactoring

[ErisDS]

We have a few goals of restructuring the codebase that we have agreed on but aren't documented anywhere. This issue serves as a place to document what we plan to do & as a place to document new utils and concepts that are intended to make our lives easier and help us move faster.

---

Lib & Lib common

Add a new lib folder, to contain all of the shared libraries in Ghost.

Underneath this, a new common folder, which contains the libraries that are used everywhere:

• events
• errors
• i18n
• logging

These 4 things need to be exposed to the whole of Ghost including theme helpers, apps & adapters.

They are are all effectively "read only" things - it's not possible to affect behaviour by using these libraries.

Other things that might live under lib, would be an apps folder that contains all the code which _manages_ apps, whereby the top level apps folder contains Ghosts built-in "internal" apps.

Services

There is a second type/set of shared libraries in Ghost, which I would call "services". These cannot be exposed to apps/adapters/themes directly without creating security concerns or permissions problems:

• xmlrpc/slack/zapier "ping" services
• mail
• auth
• sitemaps
• rss
• permissions?
• config?

In the near future this might also feature a new url and/or routing service.

Services might end up having some sort of registry in future.

Clearing up data

The data folder is a crutch. It's full of all kinds of weird stuff that has always needed a proper home. Not sure how we're going to move forward with the meta folder just yet, but we know this is a nightmare.

Many of the these things will get moved to services over time.

A fresh look at utils

We've bunged an awful lot of stuff in "utils" over the years. Some of it should be there, some of it should probably be in a service (url stuff). Some of it should belong TO a library or service, and some of it can be farmed out to either be a dependency or by using a new dependency.

Tests

We want to focus mainly on unit testing and _either_ the route-based functional tests, or the integration tests. There's a whole bunch of tests which can/should be removed or replaced. The aim is to make tests FAST so we can ship FAST.

The utilities & fixtures around tests need to be similarly split up so that different types of test have different types of utilities, and that fixtures for specific bits of data or config are easier to find / write.

Where we have defined data structures, we can move towards using more custom should assertions - E.g. for testing that API responses or resources look how we expect them to look.

---

There's not going to be any one big push towards doing this, but it is expected that PRs and commits go by that make small movements towards these goals.

---

Rough task list

(so we know when we can close this)

Structure:

• [x] Moved to having a lib
• [x] Moved to having a services folder and most of the "interesting" stuff lives in there... [this is almost done, i am not sure right now what else to move to services]
• [x] Add lib/common with the common errors, events etc & a special way to require these
• [x] Get rid of the utils folder, finding these things a better home
• [ ] Data folder doesn't contain random things
• [x] New web folder for all the expressy stuff

Test refactoring:

• [x] try to speed up routing tests (start/stop behaviour of the server and db reset)
• [x] split tests into acceptance, regression and unit tests
• [x] cronjob for regression tests
• [ ] re-work acceptance tests
• [ ] re-work regression tests
• [ ] tidy up bad unit tests

Tools:

• [x] Switch to eslint
• [x] Upgrade sinon
• [x] Upgrade as much as possible test deps
• [x] re-use request utility everywhere (e.g. https://github.com/TryGhost/Ghost/blob/bffb3dbd90bfe572798f81645e3cea908570b978/core/server/api/webhooks.js#L17-L17)

[ErisDS]

@kirrg001 loosely related thing that I wanted to have a discussion about, is the structure of tests going forward.

I have a couple of points here which are about doing a little bit of upfront work in order to, long term, significantly reduce time & complexity of testing:

Question 1: What sort of tests should we be writing?

Below is a table I generated from the latest master build.

The numbers in the top are how _many_ of that type of test we have, the numbers in the columns are how long mocha reports the tests took, the total is what Travis says:

| Node | DB | Routes (307) | Module (4) | Unit (1514) | Integration (554) | Total
|---|---|--:|--:|--:|--:|--:|
| v8 | sqlite | 2m | 7s | 15s | 3m | 6:53 |
| v8 | mysql | 3m | 8s | 18s | 3m | 7:19 |
| v6 | sqlite | 3m | 9s | 20s | 3m | 8:52 |
| v6 | mysql | 4m | 12s | 29s | 4m | 8:39 |
| v4 | sqlite | 4m | 11s | 26s | 4m | 11:55 |
| v4 | mysql | 4m | 11s | 21s | 3m | 8:56 |

It is good to see they take less time in newer versions of node, but the fundamental point is this:

We have both concepts of integration AND route tests.

• Integration are like unit tests in that they test specific things, but they are allowed to involve the database and more layers of the app than a unit test would.
• Functional route tests use supertest/superagent to boot Ghost up and test full stack behaviours.

Route tests are slower and more involved than integration tests, but both are ridiculously, significantly slower than unit tests.

IMO, we should focus on writing unit tests - we have many many of these and they still take under a minute to run. IMO we can have up to 1 minute of unit tests and life is still good.

We should then, probably use a single style of deep testing - either integration or route tests. Or we should have both but for very specific things? i.e. API -> Route tests, "database behaviour" (import, export, migrations) -> integration.

I am very much up for deleting a large section of tests if we can identify which ones are not really useful.

imagine if we only had one of the routes/integration tests - we would cut our test time by a half! 🎉

Question 2: What tools should we be using?

I think, if we narrow down the kinds of tests we write, we can expand the tools we have.

I did a quick PR here: https://github.com/TryGhost/Ghost/pull/8206 where we discussed some things.

But a short list for me would be:

• 
  
  1. Upgrade sinon, the newer version has better promise support and is easier to work with
  
  
• 
  
  1. Switch to using ESLint! Have only 1 linting tool, that everyone understands better
  
  
• 
  
  1. Have some sort of tool for doing server-folder requires
  
  
• 
  
  1. Introduce a test overrides file and make use of globals for sinon, should etc and probably more things
  
  
• 
  
  1. Make much better use of custom should assertions, especially for testing the API
  
  
• 
  
  1. Clean up and separate our test utils, so that we have clear tools for unit tests, for other tests, specific tools for munging data, faking authentication, and other clear tasks.
  
  

In short, we need to get out of our own way in the tests. Make them run faster, make them easier to write. Invest in tools so that we can test the RIGHT things FASTER. Don't bother testing the wrong things!

---

I would like to discuss this a little bit and then maybe start a wiki page that has guidelines that say "this is how we write tests", so that as we move forward we move towards clear goals. It's easier to work that way than to try to change all of the code base & rewrite all of the tests all in one go.

[kirrg001]

Good summary 👍

Route tests are slower and more involved than integration tests, but both are ridiculously, significantly slower than unit tests.

The routing tests are that slow, because each test file (and sometimes even each test case) resets the database and starts/stops the ghost server. That eats lot's of time. This should be improved by changing the way we ensure that the test case can rely on a running ghost server. For example: By default we start the server once, as soon as a test has to restart or clean the database, it tells the test utility to restart the server. Furthermore we should/could use truncate instead of a hard db reset.

We should then, probably use a single style of deep testing - either integration or route tests. Or we should have both but for very specific things? i.e. API -> Route tests, "database behaviour" (import, export, migrations) -> integration.

I think both types of testing are important. Personally, routing tests are more important, because we ensure that Ghost works from a user perspective (e.g. also user flows, url behaviour). Even that they have the disadvantage that if a test fails it's harder to figure out the reason why the test failed, because it affects the whole system. But we have added better logging and since it's easier to find out the reason.

But integration tests are important as well, as soon as we have to test connected components, which are easier to test with integration tests e.g. timezone changes and we expect a specific behaviour. But i agree, we overuse integration tests for e.g. model and api behaviour. Same for routing tests, we used to copy test cases to ensure all kinds of edge cases work.

As both types are equally slow and if we can improve the speed of the routing tests, i would like to suggest that only one type tests the model/api layer. And personally i think the routing tests should cover that. All edge cases belong to unit tests if possible.

I can think of the following rule:

1. Routing tests
   Should ensure the basic functionality of our frontend and JSON api. No edge case testing (e.g. we have 6 routing tests which test the behaviour of the formats property).

2. Integration tests
   Should ensure that components play nicely together. I suggest a minimal usage, because refactorings/architecture changes can easily break these tests.

Here is my suggestion for a task list:
EDITED: List was ported to the issue description.

Each subtask allows to remove duplicated or unimportant tests.

[ErisDS]

Agree on all of that 👍

So we have a plan to move forward in the tests 🎉 , have a plan is half the problem solved 😁

---

Back in the /server/ folder, I think that all of the stuff to do with express apps (and possibly also middleware) should be grouped together under a top level folder. I've been wondering what to call it cos we can't call it "apps" as they are express apps, not internal ghost apps... and decided web probably makes most sense.

So the /admin/ folder, /site/ (used to be blog) folder, the app.js & routes.js file from/api/ and also the current /server/app.js file, that should probably be parent-app.js all live together.

Note 1: this probably needs a bit more work on the API folder first, cos the stuff that's in index.js should really be middleware and work a bit differently.
Note 2: the top level views folder should really be inside of theme...

[ErisDS]

@kirrg001 I removed the confusing image from the previous comment and very quickly moved everything around on a branch:

Can take a look here: https://github.com/ErisDS/Ghost/tree/structure-spike/core/server

Not suggesting it's perfect, but it's a rough idea.

[kirrg001]

I'll work on the agreed test env improvements this and next week 👍

[kirrg001]

FYI: I am first concentrating on the server folder refactoring and then continue with the test env, otherwise i have to tidy up the tests twice 😎

[kirrg001]

I think it's the best if we replace the codebase to use the following lib packages for now.

const common = require('../lib/common)
const lib = require('../lib)
const image = require('../lib/image)

As soon as we drop Node v4, we can simply transform them into 🙊

const {errors,i18n} = require('../lib/common)
const {common} = require('../lib)

I need to come up with some better folder/packages naming for the folders inside lib, because otherwise we end up with using promise or fs, which is super confusing (see). I think the goal should be to have all the lib files grouped.

[kirrg001]

Every utility which is used in a single place, belongs to a local utils file. We have local utilities for api, helpers etc.

This has the advantage of reducing the requires from the root directory level of the server and makes a package independent from the rest of the application. There is no need to add a single function to a lib folder (except it fulfils a general functionality for the whole project and is used more than once or it's obvious that it's a useful lib which could be used in multiple places). The disadvantage is that the overview of available (local) utility functions is lower. In worst case it could happen that somebody adds a utility function twice.

libs should be a global place for general function/package sets.
utils (local utils) per package do special, local things for the target package. They can be unit tested and live in the scope of the package.

[ErisDS]

Really exciting seeing this come to life! Your point about the utils is 👍 👍 - I only very quickly went through to try to get rid of that folder in the example.

[kirrg001]

If anyone is interested in helping with the following task

• go over integration model/api tests and figure out which of them are real unit tests (e.g. test passing staticPages with different values is a unit test, because we don't test the database, we test the logic in the model layer) + see if tests are covered by routing tests

I already started with this a little while ago, see PR, but wasn't able to continue on it.

Leave a message here :)

[ErisDS]

Old table (minus module tests)

| Node | DB | Routes (307) | Unit (1514) | Integration (554) | Total
|---|---|--:|--:|--:|--:|
| v8 | sqlite | 2m | 15s | 3m | 6:53 |
| v8 | mysql | 3m | 18s | 3m | 7:19 |
| v6 | sqlite | 3m | 20s | 3m | 8:52 |
| v6 | mysql | 4m | 29s | 4m | 8:39 |
| v4 | sqlite | 4m | 26s | 4m | 11:55 |
| v4 | mysql | 4m | 21s | 3m | 8:56 |

Updated version, based on this recent build which fixed a broken test.

| Node | DB | Routes (530) | Unit (2106) | Integration (328) | Time
|---|---|--:|--:|--:|--:|
| v10 | sqlite | 4m | 34s | 4m | 11:8m |
| v10 | mysql | 4m | 32s | 3m | 9:09 |
| v8 | sqlite | 4m | 30s | 3m | 10:12 |
| v8 | mysql | 4m | 32s | 3m | 9:14 |
| v6 | sqlite | 5m | 33s | 4m | 10:39 |
| v6 | mysql | 5m | 44s | 4m | 11:25 |

Total time: 18 min 28 sec

Some observations:

• Node v10 is a bit slower than Node v8
• Unit tests are taking almost twice as long as 1 yr ago
• Time on BOTH integration and route testing has gone up by ~1min

The increased time on unit tests would be fine if we were seeing a significant reduction in the route or integration tests. Instead, we've got increases across the board.

I know we have more code and more features, but this is getting out of control. It feels like tests are getting in our way more than they are helping.

Using the Node v10 SQLite build as the gold standard, any test that runs at over 1000ms needs to be rewritten. Any unit tests which use the database must also be rewritten.

Any remaining > 1000ms test, or unit tests using the DB will be deleted on 14th Jan 😬

[kirrg001]

Total time: 18 min 28 sec

If I look at the last passed 10 builds, _the total time to wait is between 11-13mins._

This is not brilliant, absolutely, but:

I know we have more code and more features, but this is getting out of control. It feels like tests are getting in our way more than they are helping.

I just picked two random commits from 2017 (build1, build2)

Total Time 12mins and 13mins 😁

We should definitiely spend some time asap to finish up the tasks from this issue to improve:

• speed
• maintenance
• test seperation (what belongs to unit, what belongs to functional, what belongs to integration)

---

Using the Node v10 SQLite build as the gold standard, any test that runs at over 1000ms needs to be rewritten.

Agree 👍 Added a subtask.

Any unit tests which use the database must also be rewritten.

This is already tracked as task 👍

[kirrg001]

I am currently working on figuring out how our acceptance tests will look like. I am playing locally with a first test which uses a new setup for acceptance tests.

Some problems i discovered

• we start the Ghost server by requiring the index.js file, this can trouble with other tests, because of event listeners etc. we don't use a separate process
• acceptance tests always insert the same test data, slows down testing
• some acceptance tests require files in Ghost (coupling!)
• we can't run tests in parallel
• we currently have too many acceptance tests
• we use one big utility folder for everything
• we use too many single assertions
• the API response comparison depends on the model schema

Approaches

• outsource & re-use Ghost-CLI utility to start/stop a local process
• create test databases, which are automatically used when you start up the testing server
• be able to extend or re-generate test databases
• do not require any file in Ghost
• create a basic use case set and move all other tests to regression tests
• make more usage of custom assertions
• create API output schema

[kirrg001]

Note for re-working regression tests:

We should not keep any tests in regression tests which affect our daily work.

e.g. you add tables to our database schema (!). It is fine if the acceptance test exporting a db and assert number of tables (!)

The regression tests should be independent from general updates to Ghost.
Otherwise, we still need to run yarn test:regression to ensure that we won't break them by accident. And that would be annoying.

[kirrg001]

Reduced, moved, cleaned acceptance tests ✅Travis is down to 2-3mins.

@gargol will do the same for regression tests.

It's important that we first reduce the tests, before we change our utility or our assertions (less work).

[kirrg001]

I had a blog running on port 2369 and I was unable to run my tests anymore. I was trying to figure out for 10minutes why it's not working. There was no helpful debug output and I haven't received a proper error.

Would be great if we could resolve this pain 🤣

[naz]

After cleaning up redundant/slog tests, my current goal is to improve our regression test suite. After some analysis done yesterday here are the conclusions that came up with.

API suite has rather comprehensible structure. Think it should follow similar setup and rules as acceptance tests but with more specific state setup. Will need to be converted once acceptance suite is reworked. Don't see a need to reinvent the wheel for them :)

The rest of the suite has no clear structure and has a very different state setup/assertion techniques. For example, site_spec mocks express and doesn't operate on supertest built-in assertions. Some tests operate strictly on model layer (/models, /migrations), which is very coupled as for regression test. Some suites are using cheerio to check response content but it's inconsistent around what is actually checked.

List of tests non-API suites and possible refactors:

• models - should be rewritten into API regression tests. Very specific cases should be moved to unit tests. Goal - remove them completely?
• apps/subscribers - should move into 'site' suite or burned completely (as this module is going away soon). Names of test cases are non-descriptive e.g: [success] (should be checked where this convention is used elsewhere and modified)
• exporter - move to API tests
• importer - touches service/model layer, very specific usecase. should stay as is
• migrations - it is not really a 'migration' spec, it's a check to see if default site fixtures populate the db in expected way. most of it are permission checks. tightly coupled with models layer. probably could be grouped together with importer (will stay as a separate test but grouped for similar style).
• site
  
  
  
  • dynamic_routing - tests hight level redirects, status codes for collection index, collection entry, /tag, /author etc. Similar assertion setup style like in API tests. Has skipped suites which have inserts (timeout issues). doesn't seem to need changes now.
  
  
  • frontend - very similar to dynamic_routing suite
  
  
  • site - ~3.5K line behemoth... needs to be broken down into pieces first. 1st by api version (no need to touch v01 in the future), 2nd might need to be broken down by areast which it tests for better readability/maintainability (e.g colletions, routes). It's very hard to interpret it atm. Needs a strategy on what to do with it.
  
  
  • url_service - operates on API v0.1 (needs v2), looks like is mostly well covered by unit tests. Could be convert to API regression test, and set up the state using /settings/routes/yaml endpoint.
  
  
• update-check - to be converted into Unit test (most of it's dependencies are mocked already)

[kirrg001]

Let's discuss this ^ on Friday 👍

[naz]

@kirrg001 if it's ok, would be best to talk through it on Thursday morning your time as I'm flying on Friday :wink:

[kirrg001]

We had a chat about the test env improvements and we realised that we want to much at once and we need to set smaller goals.

Goals for next Monday

• Naz will reduce the model regression tests to its need
• Kate will provide a utility in Ghost-utils to start/stop an instance

We will set tiny goals every week, which are achievable on a Monday.

[naz]

Had a stab at the reduction of model regression tests today. Was able to fairly easy port settings model suite into unit tests by mocking db access with mock-knex. Tried taking a similar approach with posts model suite, but because the logic is far more complex mocking db layer became complicated quickly. Reduced and ported to API regression tests, some of the cases for posts model regression suite.

Couple thoughts regarding a further reduction of posts model regression suite:

1. We need to refactor the post model methods like onSaving into smaller functions so it becomes unit testable. This will allow porting validation test cases and possibly other cases.
2. Cover collision detection suite with unit tests for collision detection plugin
3. Multiauthor Posts - can be reduced to single API regression test with couple post and few authors, there's no need to do 25 post/author insertions
4. Discuss further what other possible approaches to test model events. Mocking db layer is too complicated.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[naz]

Not stale