Skip to content

test: migrate fake-server to express - #2265

Merged
2 commits merged into
masterfrom
test/migrate-to-express
Sep 23, 2021
Merged

test: migrate fake-server to express#2265
2 commits merged into
masterfrom
test/migrate-to-express

Conversation

@ghost

@ghost ghost commented Sep 21, 2021

Copy link
Copy Markdown

Why

It seems Restify is unable to correctly process the JSON payloads we send, at least going through snyk monitor. This blocks tests for #2264.

The issue seems to be a race condition between Restify’s bodyReader (which assigns req.body) and jsonBodyParser (which converts req.body to a js object). jsonBodyParser receives an empty req.body and fails to parse it. However, bodyReader clearly assigns the correct req.body — eventually. No idea if/why/how the other test endpoints are working. They look identical.

Restify is pretty inactive and we get deprecation warnings for it so instead of digging further, I'll try migrating us to Express and see if that fixes it. We want to use Express anyway as it's a lot more popular so has a familiar API.

I can confirm that these changes fixes the body parsing issues I had for #2264.

Changes

The main change is in fake-server.ts. I've migrated restify to express.

Due to how we previously assigned additional variables and functions on the server object, it was difficult to make the code in fake-server typesafe. So I've changed a few things to make that possible and avoid the inheritance we previously relied upon for FakeServer which coupled us to Restify.

  • Variables like server.requests and server.depGraphResponse are no longer directly exposed outside the closure. You'll need to use the new setter methods.
    • We should only expose functions so that future migrations are easier.
  • Created wrappers around server.listen and server.close to avoid changing heavily used public APIs.
    • In the future, we can change these wrappers to use promises and get rid of the callbacks in our test hooks.

Restify cannot be removed yet. We need to migrate tests using the old cli-server.js to fake-server.ts. This isn't trivial, it needs some authorisation logic migrated over too. I did remove @types/restify as cli-server.js isn't TypeScript.

I've also fixed a test in fix.spec.ts which was relying on fake-server's previously broken behaviour. Now fake-server is setup in the test to explicitly return a 500. While I was there I migrated all of the tests in that file to use async/await instead of callbacks. I also adding a positive test case so that it's easier to see what success looks like.

Side Fixes

I've fixed the dangerfile to properly list all known problematic files at once. Previously it was only returning the first file. I've also excluded .js files as those can't use import/export syntax (which includes the dangerfile itself).

Related, I've fixed the import/export issues with the files DangerJS flagged.

@ghost
ghost requested review from a team as code owners September 21, 2021 15:43
@ghost
ghost requested review from a team and JamesPatrickGill September 21, 2021 15:43
@ghost
ghost marked this pull request as draft September 21, 2021 15:44
@ghost
ghost removed the request for review from JamesPatrickGill September 21, 2021 15:44
@ghost ghost mentioned this pull request Sep 21, 2021
1 task
@github-actions

github-actions Bot commented Sep 21, 2021

Copy link
Copy Markdown
Contributor
Messages
📖

This PR will not trigger a new version. It doesn't include any commit message with feat or fix.

Generated by 🚫 dangerJS against 2cc7d52

@ghost
ghost force-pushed the test/migrate-to-express branch 15 times, most recently from aee4417 to c2c8422 Compare September 22, 2021 17:12
@ghost
ghost force-pushed the test/migrate-to-express branch from c2c8422 to 5ed7ddc Compare September 22, 2021 17:22
@ghost
ghost force-pushed the test/migrate-to-express branch from 5ed7ddc to 2cc7d52 Compare September 22, 2021 17:26
@ghost
ghost marked this pull request as ready for review September 22, 2021 17:29
@ghost
ghost self-requested a review September 22, 2021 17:29
@ghost
ghost requested a review from ipapast September 22, 2021 17:29

@ipapast ipapast left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get Port function for CloudConfig is fine according to the proposed changes ✅

@maxjeffos maxjeffos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great

Comment thread test/acceptance/fake-server.ts
@ghost
ghost merged commit 73be6a6 into master Sep 23, 2021
@ghost
ghost deleted the test/migrate-to-express branch September 23, 2021 16:02
@ghost ghost mentioned this pull request Dec 2, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants