implement the build-and-test command #1595
Conversation
src/commands/synthetics/__tests__/config-fixtures/config-with-all-keys.json
Outdated
Show resolved
Hide resolved
| const port = 4000 | ||
| const requests: string[] = [] | ||
| const server = http | ||
| .createServer((req, res) => { | ||
| requests.push(String(req.url)) | ||
| res.writeHead(200, {'Content-Type': 'application/json'}) | ||
| res.end( | ||
| JSON.stringify({ | ||
| status: 'success', | ||
| publicPrefix: 'prefix/', | ||
| }) | ||
| ) | ||
| }) | ||
| .listen(port) |
There was a problem hiding this comment.
nit: Instead of creating a server (which bring some complexity to the test), you could mock the axios.get call, or use something like nock.
There was a problem hiding this comment.
Are you concerned with the complexity reading the test implementation, or executing the test?
I understand it's bringing a bit of complexity in both, but given it allows to remain close to the expected implementation, and the test implementation remains quite straightforward, I thought it'd be acceptable.
There was a problem hiding this comment.
It was mostly about the complexity of the implementation, and the overhead added to its execution.
For instance, it's quite difficult to create more test cases if needed, like different responses, without having to implement a full fledge server, with features and all. Increasing further the complexity and the overhead.
In the end, it doesn't really add any value to the test, it's used as a tool to assess that the command is fetching, and behave as expected given the server's responses (which is currently only tested with a single response).
Tool that could be replaced by a simpler one, like a mock or nock, which would give us more opportunities to easily test more situations, without adding complexity nor overhead.
Somehow also related to the previous comment about spawn.
Won't block for this though, it was mostly just a nit 👍 for testing perfs and DX.
Drarig29
left a comment
Drarig29
left a comment
There was a problem hiding this comment.
Mostly LTGM, left some comments
Datadog ReportBranch report: ✅ 0 Failed, 1329 Passed, 0 Skipped, 1m 48.26s Total duration (30.37s time saved) |
yoannmoinet
left a comment
yoannmoinet
left a comment
There was a problem hiding this comment.
Overall looks good to me.
I still have some reservations on the tests not mocking the side systems (shell and server) in order to better focus on the actual command's implementation and responses to specific use cases.
But like I said in my previous comments, I won't block for that.
What and why?
This PR implements the
synthetics build-and-testcommand.Provided with a
buildCommand, and an optionalbuildPluginPort(defaults to 4000) options, this command runs the build command, wait for a successful build, and a dev server spawned by the Datadog build-plugin, to run Synthetics tests against this dev server using the tunnel.Except the unit tests, this command hasn't been tested properly yet. It'll be done later.
How?
The
build-and-testcommand spawns the build command, and regularly request the expected dev server (e.g.https://localhost:4000) until the build is advertised as successful. Then it runs the tests.Review checklist