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

tests(clustering/rpc): add cases for sync.v2.get_delta #14151

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

chronolaw
Copy link
Contributor

@chronolaw chronolaw commented Jan 14, 2025

Summary

KAG-6179

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
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix #[issue number]

@github-actions github-actions bot added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Jan 14, 2025
@chronolaw chronolaw marked this pull request as ready for review January 14, 2025 05:31
@chronolaw chronolaw force-pushed the tests/mock_v2_get_delta branch from d994317 to b646fd4 Compare January 14, 2025 10:59

-- dp logs
helpers.pwait_until(function()
assert.logfile(name).has.line(
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of assertion already has timeout as the input parameter, so the pwait_until is not required.

assert.logfile().has.line("\\[file-log\\] failed to open the file: " ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about it, if assertion fails wait_until may crush, so pwait_until is a better choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked the code in spec/internal/wait.lua, pwait_until is different from wait_until, it can deal with assertion failings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm not clear enough, assert.logfile().has.line(pattern, boolean, timeout), we can pass the timeout into this assertion, so we don't need to warp it with wait_until or pwait_until.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got, let me try it.

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 works, but the tests take more time than pwait_until, so my opinion is keeping it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee size/L skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants