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:

• Moved to having a lib
• 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]
• Add lib/common with the common errors, events etc & a special way to require these
• Get rid of the utils folder, finding these things a better home
• Data folder doesn't contain random things
• New web folder for all the expressy stuff

Test refactoring:

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

Tools:

• Switch to eslint
• Upgrade sinon
• Upgrade as much as possible test deps
• re-use request utility everywhere (e.g.

  Ghost/core/server/api/webhooks.js

  Line 17 in bffb3db

  // TODO: Use the request util. Do we want retries here?

  )

[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: #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
• 2. Switch to using ESLint! Have only 1 linting tool, that everyone understands better
• 3. Have some sort of tool for doing server-folder requires
• 4. Introduce a test overrides file and make use of globals for sinon, should etc and probably more things
• 5. Make much better use of custom should assertions, especially for testing the API
• 6. 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]

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 😉

[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]

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