Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(*): implement new HTTP mocking #10885

Merged
merged 2 commits into from
May 23, 2023
Merged

test(*): implement new HTTP mocking #10885

merged 2 commits into from
May 23, 2023

Conversation

StarlightIbuki
Copy link
Contributor

@StarlightIbuki StarlightIbuki commented May 17, 2023

Summary

We have 3 types of implementation for HTTP mocking, each of which has some issues.

This PR introduces a new implementation to unify them with a dedicated Nginx instance.

It allows users to use the same logic as start_kong fixture. And it collects requests/responses/errors for later assertions on them.

The collected information is then fetched from a debugging port, and a set of assertion APIs handles the fetching process.

It's also possible to extend this solution to support clients-to-server debugging API, e.g, to send a piece of text as the response body for the next request.

Checklist

  • The Pull Request has tests
  • There's an entry in the CHANGELOG (Should we?)

Full changelog

  • New HTTP mocking implementations, including:
    • Logic to create and manage the Nginx server instance;
    • API to create an HTTP client to the server;
    • APIs to make assertions on requests, responses, and errors;
  • Tests of the new implementation;
  • Deprecating start_kong fixture, http_server, mock_http, and https_server;
  • An example of using the new implementation.

Issue reference

Fix KAG-1148

Todo after this merged

Migrate current uses of start_kong fixture, http_server, mock_http, and https_server.

Copy link
Contributor

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

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

Could you please move the tests from spec/helpers/http_mock_spec.lua to spec/02-integration/01-helpers?

Copy link
Contributor

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

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

Please use the new mocking server replace some old code that used the old mocking servers.

spec/helpers/http_mock/asserts.lua Outdated Show resolved Hide resolved
spec/helpers/http_mock/template.lua Outdated Show resolved Hide resolved
@StarlightIbuki StarlightIbuki force-pushed the test/new-http-mock branch 2 times, most recently from 97e1b0b to fbd28cb Compare May 22, 2023 06:46
@StarlightIbuki StarlightIbuki marked this pull request as ready for review May 22, 2023 06:50
@StarlightIbuki StarlightIbuki requested a review from ADD-SP May 22, 2023 06:50
@StarlightIbuki StarlightIbuki force-pushed the test/new-http-mock branch 2 times, most recently from 315ec03 to b6a439e Compare May 22, 2023 07:24
spec/helpers/http_mock.lua Show resolved Hide resolved
spec/helpers/http_mock/nginx_instance.lua Outdated Show resolved Hide resolved
spec/helpers/http_mock/nginx_instance.lua Show resolved Hide resolved
spec/helpers/http_mock/template.lua Outdated Show resolved Hide resolved
@StarlightIbuki StarlightIbuki requested a review from ADD-SP May 22, 2023 07:48
Copy link
Contributor

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

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

spec/02-integration/05-proxy/03-upstream_headers_spec.lua is red

@StarlightIbuki StarlightIbuki requested a review from ADD-SP May 22, 2023 08:44
Copy link
Contributor

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@hanshuebner hanshuebner left a comment

Choose a reason for hiding this comment

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

I like this proposal but I have a couple of comments and suggestions. Generally, do we have an understanding of the time that it takes to start up nginx compared to the thread and coroutine based mock servers?

spec/helpers/http_mock.lua Outdated Show resolved Hide resolved
spec/helpers/http_mock.lua Outdated Show resolved Hide resolved
spec/helpers/http_mock.lua Outdated Show resolved Hide resolved
spec/helpers/http_mock.lua Show resolved Hide resolved
Comment on lines +85 to +117
local function register_assert(name, impl)
eventually_MT["has_" .. name] = function(self, ...)
return eventually_has(impl, self.__mock, ...)
end

eventually_MT["all_" .. name] = function(self, ...)
return eventually_all(impl, self.__mock, ...)
end

local function reverse_impl(session, ...)
local ok, err = pcall(impl, session, ...)
if ok then
error("we don't expect that: " .. (name or err), 2)
end
return true
end

eventually_MT["has_no_" .. name] = function(self, ...)
return eventually_all(reverse_impl, self.__mock, ...)
end

eventually_MT["not_all_" .. name] = function(self, ...)
return eventually_has(reverse_impl, self.__mock, ...)
end
end

for name, impl in pairs(build_in_checks) do
register_assert(name, impl)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks cool and would probably be something to add to helpers.lua or live in a separate module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have this:

local function eventually(state, arguments)

Actually, the design is inspired by assert.eventually.

This logic is coupled with self.__mock and get_all_records(). Maybe we will find a way to decouple them.

spec/helpers/http_mock/asserts.lua Outdated Show resolved Hide resolved
spec/helpers/http_mock/debug_port.lua Outdated Show resolved Hide resolved
spec/helpers/http_mock/debug_port.lua Outdated Show resolved Hide resolved
spec/helpers/http_mock/template.lua Show resolved Hide resolved
@StarlightIbuki
Copy link
Contributor Author

StarlightIbuki commented May 23, 2023

I like this proposal but I have a couple of comments and suggestions. Generally, do we have an understanding of the time that it takes to start up nginx compared to the thread and coroutine based mock servers?

Starting a vanilla Nginx is really quick. I tried the test 03-http_mock_spec.lua on my own laptop and it takes less than 4s to run 15 tests (if we set a very small timeout for those waiting tests).

spec/helpers.lua Outdated Show resolved Hide resolved
spec/helpers.lua Outdated Show resolved Hide resolved
spec/fixtures/https_server.lua Outdated Show resolved Hide resolved
spec/helpers.lua Outdated Show resolved Hide resolved
Copy link
Contributor

@hanshuebner hanshuebner left a comment

Choose a reason for hiding this comment

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

just some minor nits, please

spec/helpers/http_mock.lua Outdated Show resolved Hide resolved
spec/helpers/http_mock.lua Show resolved Hide resolved
spec/helpers/http_mock/asserts.lua Show resolved Hide resolved
StarlightIbuki and others added 2 commits May 23, 2023 17:28
Fix KAG-1148

apply suggestions

Co-authored-by: Chrono <chrono_cpp@me.com>
Co-authored-by: Hans Hübner <hans.huebner@gmail.com>
@hanshuebner hanshuebner merged commit 068b318 into master May 23, 2023
@hanshuebner hanshuebner deleted the test/new-http-mock branch May 23, 2023 10:09
chobits added a commit that referenced this pull request Nov 29, 2023
This commit backports HTTP mocking to release/3.2.x, including the
following PRs from master branch: #10885, #11009, #11182 and #11331.
windmgc pushed a commit that referenced this pull request Nov 29, 2023
This commit backports HTTP mocking to release/3.2.x, including the
following PRs from master branch: #10885, #11009, #11182 and #11331.
chobits added a commit that referenced this pull request Nov 30, 2023
This commit backports HTTP mocking to release/3.2.x, including the
following PRs from master branch: #10885, #11009, #11182 and #11331.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants