Speed up loop.spec.ts
#15674
Speed up loop.spec.ts
#15674
Conversation
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
|
|
||
| [httpd] | ||
| socket_options = [{nodelay, true}] | ||
|
|
||
| [couchdb] | ||
| single_node = true | ||
|
|
||
| [cluster] | ||
| n = 1 | ||
| q = 1 |
There was a problem hiding this comment.
I went through https://docs.couchdb.org/en/stable/maintenance/performance.html and enabled a few things. I don't think it had much of a performance impact, but it did get rid of some errors in the logs so I'm going to leave it in.
| @@ -290,8 +290,7 @@ describe("/automations", () => { | |||
| await setup.delay(500) | |||
| let elements = await getAllTableRows(config) | |||
| // don't test it unless there are values to test | |||
| if (elements.length > 1) { | |||
| expect(elements.length).toBeGreaterThanOrEqual(MAX_RETRIES) | |||
| if (elements.length >= 1) { | |||
There was a problem hiding this comment.
This was previously relying on the automation created in this test chaining with itself. Under normal circumstances, only 1 row should be created.
There was a problem hiding this comment.
The quota stuff in this file, and a few others, is leftover from my attempt to speed up tests by disabling quota checking everywhere except tests that relied on it. I've gotten rid of it now but liked these changes, so kept them.
| @@ -20,9 +20,12 @@ export interface TriggerOutput { | |||
|
|
|||
| export interface AutomationContext { | |||
| trigger: AutomationTriggerResultOutputs | |||
| steps: [AutomationTriggerResultOutputs, ...AutomationStepResultOutputs[]] | |||
| stepsById: Record<string, AutomationStepResultOutputs> | |||
| steps: Record< | |||
There was a problem hiding this comment.
Previously we would track context in this shape and before each time we used it, we'd call a function called prepareContext that slightly transformed it. This ended up being quite expensive, so I made it such that the transformation is no longer needed.
| _stepIndex: number | ||
| _error: boolean |
There was a problem hiding this comment.
_stepIndex tracks what numerical index to use next for step output (e.g. for {{ steps.0.output }}), and _error tracks whether a step has failed. This removes the need for us to loop over all of the output to check.
| } else if (env.isTest()) { | ||
| return 0 |
There was a problem hiding this comment.
This was the key to solving the reason loop.spec.ts has been timing out. We had lots of automations that triggered themselves, which was causing havoc in the background while other tests were running. This prevents that, unless chaining is explicitly enabled in the app metadata.
| @@ -23,6 +23,6 @@ nock.enableNetConnect(host => { | |||
|
|
|||
| testContainerUtils.setupEnv(env, coreEnv) | |||
|
|
|||
| afterAll(() => { | |||
| afterAll(async () => { | |||
There was a problem hiding this comment.
Leftover from attempt to disable quota during tests, left in by accident. Can change it back if you want but I don't think it causes any harm.
| try { | ||
| return await req | ||
| resp = await req |
There was a problem hiding this comment.
Did this to make debugging easier.
| const job = cloneDeep(this.job) | ||
| delete job.data.event.appId | ||
| delete job.data.event.metadata | ||
| const data = cloneDeep(this.job.data) |
There was a problem hiding this comment.
This was another big part of why automation tests became slow. this.job contains a reference to InMemoryQueue, which in turn has a reference to _messages which contains all automation queue messages fired during tests. Cloning it in full was expensive.
| @@ -165,6 +162,9 @@ export class InMemoryQueue<T = any> implements Partial<Queue<T>> { | |||
| opts, | |||
| } | |||
| this._messages.push(message) | |||
| if (this._messages.length > 1000) { | |||
| this._messages.shift() | |||
There was a problem hiding this comment.
Can this have a side effect? Could we not remove it when processed?
There was a problem hiding this comment.
Unfortunately I don't think we can remove it when the job finishes, because cron jobs will need to be triggered multiple times.
It may have a side effect, but I tried to think of a sufficiently large number so as to prevent side effects.
Description
This should solve the timeouts we've been seeing with
loop.spec.tsrecently. I've left comments inline explaining, it was quite a journey to debug.