Skip to content

Conversation

@Revolyssup
Copy link
Contributor

Description

Currently we have mock tests with udp logger. This PR uses vector to replace them with real tests by checking the logs from logging server

Fixes # (issue)

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
--- grep_error_log_out
sending a batch logs to 127.0.0.1:2000
sending a batch logs to 127.0.0.1:2001
sending a batch logs to 127.0.0.1:2002
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we also need replace these ports to real log server port?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Sn0rt We can let these ones as it is, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

It’s okay, it’s better if a real udp logger is introduced to replace it

@jiangfucheng
Copy link
Member

Maybe we should add more test cases with batch_max_size > 1

@monkeyDluffy6017
Copy link
Contributor

@Sn0rt please help to review

Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

should I set the UDP port by 8125:8125/udp ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep this looks right. I made this change.

"vip": "$remote_addr"
},
"batch_max_size": 1,
"inactive_timeout": 1
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed to remove .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was added back in later commit

--- grep_error_log_out
sending a batch logs to 127.0.0.1:2000
sending a batch logs to 127.0.0.1:2001
sending a batch logs to 127.0.0.1:2002
Copy link
Contributor

Choose a reason for hiding this comment

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

It’s okay, it’s better if a real udp logger is introduced to replace it

Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Revolyssup and others added 3 commits May 19, 2023 22:30
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Co-authored-by: Tristan <jiangfucheng0914@foxmail.com>
leslie-tsang
leslie-tsang previously approved these changes May 23, 2023
Copy link
Member

@leslie-tsang leslie-tsang left a comment

Choose a reason for hiding this comment

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

LGTM

Sn0rt
Sn0rt previously approved these changes May 23, 2023
Comment on lines +351 to +356
local code, _, _ = t("/hello", "GET")
if code >= 300 then
ngx.status = code
ngx.say("fail")
return
end
Copy link
Contributor

@monkeyDluffy6017 monkeyDluffy6017 May 23, 2023

Choose a reason for hiding this comment

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

The test case's name is add plugin, but you also access the route in the test case, why not create a new test case to access?

=== TEST 10: access
--- request
GET /hello
--- response_body
passed

Copy link
Contributor Author

@Revolyssup Revolyssup May 23, 2023

Choose a reason for hiding this comment

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

@monkeyDluffy6017 renamed the tests appropriately.

Comment on lines 423 to 428
local code, _, _ = t("/hello", "GET")
if code >= 300 then
ngx.status = code
ngx.say("fail")
return
end
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto, and remember to add a blank between two blocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@monkeyDluffy6017 monkeyDluffy6017 added the wait for update wait for the author's response in this issue/PR label May 23, 2023
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
@Revolyssup Revolyssup dismissed stale reviews from Sn0rt and leslie-tsang via 68ac857 May 23, 2023 07:31
return
end
ngx.say(body)
local code, _, _ = t("/hello", "GET")
Copy link
Contributor

Choose a reason for hiding this comment

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

need a blank before this block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops I had missed it. Added now. @monkeyDluffy6017

Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
@monkeyDluffy6017 monkeyDluffy6017 removed the wait for update wait for the author's response in this issue/PR label May 23, 2023
@monkeyDluffy6017 monkeyDluffy6017 merged commit eaed6c8 into apache:master May 24, 2023
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.

5 participants