tests(clustering/rpc): rpc mock/hook #14030

StarlightIbuki · 2024-12-17T10:13:34Z

Summary

RPC needs mock tests

Checklist

The Pull Request has tests
A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md

Issue reference

Fix KAG-5551

spec/01-unit/19-hybrid/04-rpc_spec.lua

chobits · 2024-12-18T08:51:14Z

spec/helpers/rpc_mock/server.lua

+    config = {},
+  })
+
+  assert(helpers.start_kong(self))


start a mock cp to proxy client's rpc request to DP

chobits · 2024-12-18T08:52:05Z

spec/helpers/rpc_mock/server.lua

+
+  assert(helpers.start_kong(self))
+
+  self.proxy_client = client.new({


client -> mock cp --> tested DP

chobits · 2024-12-18T08:54:44Z

spec/helpers/rpc_mock/server.lua

+    self:enable_inception()
+  end
+
+  self.proxy_client.callbacks:register("kong.rpc.proxy.mock", function(proxy_id, proxy_payload)


mock cp will rpc call this callback to forward [the RPC call request from tested DP] to [client]

chobits · 2024-12-18T09:01:38Z

spec/01-unit/19-hybrid/04-rpc_spec.lua

+        server_mock = server.new()
+        assert(server_mock:start())
+
+        assert(helpers.start_kong({


TODO: add a comment to explain this is.a tested DP

spec/helpers/rpc_mock/client.lua

spec/02-integration/01-helpers/05-rpc-mock_spec.lua

spec/01-unit/19-hybrid/04-rpc_spec.lua

spec/02-integration/01-helpers/05-rpc-mock_spec.lua

spec/01-unit/19-hybrid/04-rpc_spec.lua

spec/helpers/rpc_mock/client.lua

chobits · 2024-12-19T09:59:48Z

spec/helpers/rpc_mock/server.lua

+    local prehook = self.prehooks[method] or self.prehooks["*"]
+    local posthook = self.posthooks[method] or self.posthooks["*"]


need add some comment for prehook and post hook

chobits · 2024-12-19T10:01:38Z

spec/helpers/rpc_mock/server.lua

+local helpers = require("spec.helpers")
+local client = require("spec.helpers.rpc_mock.client")
+local default_cert = require("spec.helpers.rpc_mock.default").default_cert
+


I strongly recommend you to write a callback and diagram in the comment like, or you could write a design doc

-- Diagram: -- -- mock client -> mock server -> tested DP -- -- Callback chains: -- -- func1 -- -> func2 -- -> func3 ...

otherwise other developers or reviewers find it hard to understand, today i talked with @chronolaw , he found it hard to understand what the server.lua does here.

a good way is that you could write a test case to run this mock client -> mock server -> tested DP progress, i find there is no case for mock server<>tested DP. Then we could explain the progress with that case, that might be easy to understand for reviewers.

KAG-5551

spec/internal/misc.lua

Co-authored-by: Chrono <[email protected]>

spec/01-unit/09-balancer/03-consistent_hashing_spec.lua

spec/02-integration/01-helpers/05-rpc-mock_spec.lua

spec/fixtures/custom_plugins/kong/plugins/rpc-debug/handler.lua

chronolaw · 2024-12-24T09:39:37Z

spec/helpers.lua

@@ -226,4 +226,10 @@ local wait = reload_module("spec.internal.wait")
  make_temp_dir = misc.make_temp_dir,

  build_go_plugins = cmd.build_go_plugins,
+
+  get_node_id = misc.get_node_id,


Perhaps we should not export these apis in helpers, could we use misc directly?

I suppose get_node_id has more general usages

Currently they are only used in rpc mock, we could move it to misc now and expose them in the future.

spec/helpers/rpc_mock/client.lua

chronolaw · 2024-12-24T09:40:47Z

spec/helpers/rpc_mock/client.lua

+
+
+local function client_is_connected(rpc_mgr)
+  for _, socket in pairs(rpc_mgr.clients) do


Should we use table.isempty?

I don't think it's performance critical for a test function

table.isempty is easy to understand for reviewers, and we should always write the high quality code.

Co-authored-by: Chrono <[email protected]>

chronolaw · 2024-12-25T02:08:19Z

spec/02-integration/01-helpers/05-rpc-mock_spec.lua

+
+      -- the mock should have been called
+      helpers.wait_until(function()
+        return client_version


Should we add assert(client_version == 100)?

chronolaw · 2024-12-25T02:10:56Z

spec/fixtures/custom_plugins/kong/plugins/rpc-debug/handler.lua

+
+            return res.result, res.error
+          end
+        elseif res.mock then


blank line before elseif

chronolaw · 2024-12-25T02:12:36Z

spec/helpers.lua

@@ -226,4 +226,10 @@ local wait = reload_module("spec.internal.wait")
  make_temp_dir = misc.make_temp_dir,

  build_go_plugins = cmd.build_go_plugins,
+
+  get_node_id = misc.get_node_id,


Currently they are only used in rpc mock, we could move it to misc now and expose them in the future.

pull-request-size bot added the size/L label Dec 17, 2024

github-actions bot added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Dec 17, 2024

github-actions bot assigned StarlightIbuki Dec 17, 2024

chronolaw self-requested a review December 18, 2024 07:30

chronolaw changed the title ~~tests(rpc): rpc mock/hook~~ tests(clustering/rpc): rpc mock/hook Dec 18, 2024

chronolaw reviewed Dec 18, 2024

View reviewed changes

spec/01-unit/19-hybrid/04-rpc_spec.lua Outdated Show resolved Hide resolved

StarlightIbuki force-pushed the rpc-mock branch 2 times, most recently from c79e2be to 5438bcc Compare December 18, 2024 07:57

chobits reviewed Dec 18, 2024

View reviewed changes

chronolaw reviewed Dec 18, 2024

View reviewed changes

spec/helpers/rpc_mock/client.lua Show resolved Hide resolved

StarlightIbuki force-pushed the rpc-mock branch from 10b37a0 to 8656633 Compare December 19, 2024 08:09

pull-request-size bot added size/XL and removed size/L labels Dec 19, 2024

StarlightIbuki force-pushed the rpc-mock branch from 8656633 to 8a26357 Compare December 19, 2024 08:16

StarlightIbuki marked this pull request as ready for review December 19, 2024 08:17

StarlightIbuki force-pushed the rpc-mock branch from 8a26357 to 5b3a82a Compare December 19, 2024 08:22

StarlightIbuki added the skip-changelog label Dec 19, 2024

github-actions bot added the core/clustering label Dec 19, 2024

chobits reviewed Dec 19, 2024

View reviewed changes

StarlightIbuki force-pushed the rpc-mock branch 7 times, most recently from 948243c to 0174ea2 Compare December 24, 2024 02:26

tests(rpc): rpc mock/hook

419e823

KAG-5551

StarlightIbuki force-pushed the rpc-mock branch from 0174ea2 to 419e823 Compare December 24, 2024 06:16

chronolaw reviewed Dec 24, 2024

View reviewed changes

spec/internal/misc.lua Outdated Show resolved Hide resolved

Update spec/internal/misc.lua

35b1247

Co-authored-by: Chrono <[email protected]>

StarlightIbuki force-pushed the rpc-mock branch from 240ddee to c618289 Compare December 24, 2024 09:35

f

f238be2

StarlightIbuki force-pushed the rpc-mock branch from c618289 to f238be2 Compare December 24, 2024 09:36

chronolaw reviewed Dec 24, 2024

View reviewed changes

StarlightIbuki and others added 2 commits December 24, 2024 17:44

Update spec/helpers/rpc_mock/client.lua

73d6c9f

Co-authored-by: Chrono <[email protected]>

Update spec/helpers/rpc_mock/client.lua

f12d64b

Co-authored-by: Chrono <[email protected]>

chronolaw reviewed Dec 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests(clustering/rpc): rpc mock/hook #14030

tests(clustering/rpc): rpc mock/hook #14030

StarlightIbuki commented Dec 17, 2024

chobits Dec 18, 2024 •

edited

Loading

chobits Dec 18, 2024

chobits Dec 18, 2024

chobits Dec 18, 2024 •

edited

Loading

chobits Dec 19, 2024

chobits Dec 19, 2024 •

edited

Loading

chobits Dec 19, 2024 •

edited

Loading

chronolaw Dec 24, 2024

StarlightIbuki Dec 24, 2024

chronolaw Dec 25, 2024

chronolaw Dec 24, 2024

StarlightIbuki Dec 24, 2024

chronolaw Dec 24, 2024 •

edited

Loading

chronolaw Dec 25, 2024

chronolaw Dec 25, 2024

chronolaw Dec 25, 2024


		assert(helpers.start_kong(self))

		self.proxy_client = client.new({

		local prehook = self.prehooks[method] or self.prehooks["*"]
		local posthook = self.posthooks[method] or self.posthooks["*"]



		local function client_is_connected(rpc_mgr)
		for _, socket in pairs(rpc_mgr.clients) do

tests(clustering/rpc): rpc mock/hook #14030

Are you sure you want to change the base?

tests(clustering/rpc): rpc mock/hook #14030

Conversation

StarlightIbuki commented Dec 17, 2024

Summary

Checklist

Issue reference

chobits Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chobits Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chobits Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

chobits Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chronolaw Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chobits Dec 18, 2024 •

edited

Loading

chobits Dec 18, 2024 •

edited

Loading

chobits Dec 19, 2024 •

edited

Loading

chobits Dec 19, 2024 •

edited

Loading

chronolaw Dec 24, 2024 •

edited

Loading