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

Node: Changed SCRIPT KILL test to only test the command works, not that the script is actually being killed #2629

Merged
merged 2 commits into from
Nov 7, 2024

Conversation

barshaul
Copy link
Collaborator

@barshaul barshaul commented Nov 6, 2024

Changed Node's SCRIPT KILL test to test only that the command works, not that a script is actually being killed (to test the client and not the server). Moved to SharedTests.

Issue link

This Pull Request is linked to issue (URL): closes #2625

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • CHANGELOG.md and documentation files are updated.
  • Destination branch is correct - main or release
  • Commits will be squashed upon merging.

@barshaul barshaul requested a review from a team as a code owner November 6, 2024 15:22
@barshaul barshaul force-pushed the fix_node_kill branch 2 times, most recently from de41090 to 59f786b Compare November 6, 2024 15:24
@Yury-Fridlyand Yury-Fridlyand added node Node.js wrapper testing Everything about testing labels Nov 6, 2024
@Yury-Fridlyand
Copy link
Collaborator

We discussed these tests with the team. The approach in these tests is incorrect in general - we're trying to test the server, instead of testing the client.

await expect(client1.scriptKill()).rejects.toThrow(
"No scripts in execution right now",
);

This simple check is enough to test the client, all the rest can (should) be removed.
Keep in mind that we have the same test for python and java clients too. Fix them as well.

@barshaul
Copy link
Collaborator Author

barshaul commented Nov 7, 2024

We discussed these tests with the team. The approach in these tests is incorrect in general - we're trying to test the server, instead of testing the client.

await expect(client1.scriptKill()).rejects.toThrow(
"No scripts in execution right now",
);

This simple check is enough to test the client, all the rest can (should) be removed. Keep in mind that we have the same test for python and java clients too. Fix them as well.

good point!

…at the script is actually being killed

Signed-off-by: barshaul <[email protected]>
@@ -1460,70 +1459,6 @@ describe("GlideClient", () => {
TIMEOUT,
);

it.each([ProtocolVersion.RESP2, ProtocolVersion.RESP3])(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we think that this test is not relevant at all?
Is it true for what @shohamazon is working on as well?

@barshaul barshaul merged commit c4101a9 into valkey-io:release-1.2 Nov 7, 2024
18 checks passed
@barshaul barshaul changed the title Node: Increase times for SCRIPT KILL tests to avoid flakiness Node: Changed SCRIPT KILL test to only test the command works, not that the script is actually being killed Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node Node.js wrapper testing Everything about testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants