Skip to content

Support Valkey image:latest and Redis:latest for test #258

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Nov 29, 2025
Merged

Support Valkey image:latest and Redis:latest for test #258

merged 6 commits into from
Nov 29, 2025
+19 −10

Conversation

@zunda-pixel
Copy link
Contributor

@zunda-pixel zunda-pixel commented Oct 27, 2025 •
edited
Loading

Add Valkey image v9.0 for test

  • Add Valkey image:latest(v9.0)

  • Add Redis image:latest(v8.4)

  • disable testSCRIPTfunctions() for Redis

@zunda-pixel zunda-pixel force-pushed the test-multi-valkey-version branch from 1e62f6d to afc4a44 Compare October 27, 2025 16:12
@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@b813e95). Learn more about missing BASE report.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #258   +/-   ##
=======================================
  Coverage        ?   41.00%           
=======================================
  Files           ?       97           
  Lines           ?    12296           
  Branches        ?        0           
=======================================
  Hits            ?     5042           
  Misses          ?     7254           
  Partials        ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@adam-fowler
Copy link
Collaborator

adam-fowler commented Oct 28, 2025

I don't think there is any need for multiple valkey images. There isn't enough of a difference between versions. But adding the latest redis might be useful

@zunda-pixel
Copy link
Contributor Author

zunda-pixel commented Nov 12, 2025

Sorry for the misunderstanding. I’ve added Valkey v9. Should I delete v8?

@adam-fowler
Copy link
Collaborator

adam-fowler commented Nov 13, 2025

Maybe use images valkey/valkey:latest and redis:latest.

@zunda-pixel zunda-pixel force-pushed the test-multi-valkey-version branch 4 times, most recently from fc82827 to 1c02d2c Compare November 14, 2025 09:32
@zunda-pixel
Copy link
Contributor Author

zunda-pixel commented Nov 14, 2025

func testFUNCTIONLIST(), func testSCRIPTfunctions() is failed on Redis.

disable these test, CI is succeeded
https://github.com/valkey-io/valkey-swift/actions/runs/19360342924

@adam-fowler
Copy link
Collaborator

adam-fowler commented Nov 14, 2025

Interesting FUNCTION LIST and SCRIPT LIST fail.

@zunda-pixel zunda-pixel changed the title Add Valkey image v9.0 for test Support Valkey image:latest and Redis:latest for test Nov 14, 2025
zunda-pixel
zunda-pixel commented Nov 14, 2025
View reviewed changes
Tests/IntegrationTests/CommandIntegrationTests.swift
Comment on lines +179 to +180
redis.register_function('valkey_swift_test_set', test_set)
redis.register_function('valkey_swift_test_get', test_get)
Copy link
Contributor Author

@zunda-pixel zunda-pixel Nov 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

rename to fix testFUNCTIONLIST() for Redis

zunda-pixel
zunda-pixel commented Nov 14, 2025
View reviewed changes
Tests/IntegrationTests/CommandIntegrationTests.swift Outdated

@available(valkeySwift 1.0, *)
@Test
@Test(.enabled(if: Self.isValkey))
Copy link
Contributor Author

@zunda-pixel zunda-pixel Nov 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

testSCRIPTfunctions() is enabled for Valkey not Redis
Redis has no SCRIPT SHOW

Copy link
Collaborator

@adam-fowler adam-fowler Nov 28, 2025

Choose a reason for hiding this comment

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

Instead of adding a isValkey test. Perhaps you can use HELLO to get the server name and only do the SCRIPT SHOW command test if the server name if valkey.

@zunda-pixel
set valkey:latest
f08cd37 
Signed-off-by: zunda <zunda.dev@gmail.com>
@zunda-pixel zunda-pixel force-pushed the test-multi-valkey-version branch from 4ab03be to 5902dbd Compare November 14, 2025 16:36
@zunda-pixel
add redis (#2)
4bdf840 
Signed-off-by: yazawa hiroki <zunda.dev@gmail.com>
Signed-off-by: zunda <zunda.dev@gmail.com>
@zunda-pixel
change to redis
a50d96b 
Signed-off-by: zunda <zunda.dev@gmail.com>
@zunda-pixel
add isValkey
7404c92 
remove space

Signed-off-by: zunda <zunda.dev@gmail.com>
@zunda-pixel zunda-pixel force-pushed the test-multi-valkey-version branch from 5902dbd to 7404c92 Compare November 14, 2025 16:37
@zunda-pixel
Copy link
Contributor Author

zunda-pixel commented Nov 15, 2025

@adam-fowler
I fixed tests. Please review

adam-fowler
adam-fowler reviewed Nov 28, 2025
View reviewed changes
Copy link
Collaborator

@adam-fowler adam-fowler left a comment

Choose a reason for hiding this comment

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

Sorry for the slow response here. I've been away on holiday

Tests/IntegrationTests/CommandIntegrationTests.swift Outdated

@available(valkeySwift 1.0, *)
@Test
@Test(.enabled(if: Self.isValkey))
Copy link
Collaborator

@adam-fowler adam-fowler Nov 28, 2025

Choose a reason for hiding this comment

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

Instead of adding a isValkey test. Perhaps you can use HELLO to get the server name and only do the SCRIPT SHOW command test if the server name if valkey.

@zunda-pixel
add getHello to identify server
d608f5e 
Signed-off-by: zunda <zunda.dev@gmail.com>
@zunda-pixel zunda-pixel force-pushed the test-multi-valkey-version branch from 8d384a8 to d608f5e Compare November 29, 2025 06:46
@zunda-pixel
Copy link
Contributor Author

zunda-pixel commented Nov 29, 2025

@adam-fowler
I fixed it d608f5e. is this ok?

adam-fowler
adam-fowler reviewed Nov 29, 2025
View reviewed changes
Copy link
Collaborator

@adam-fowler adam-fowler left a comment

Choose a reason for hiding this comment

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

This looks good, just needs you to run swift format

@zunda-pixel
format
cd04cf9 
Signed-off-by: zunda <zunda.dev@gmail.com>
@adam-fowler adam-fowler merged commit 83bc7a8 into valkey-io:main Nov 29, 2025
14 of 15 checks passed
@adam-fowler
Copy link
Collaborator

adam-fowler commented Nov 29, 2025

thanks for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adam-fowler adam-fowler adam-fowler left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zunda-pixel @codecov-commenter @adam-fowler