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

Adds more tests #27

Merged
merged 7 commits into from
Aug 1, 2024
Merged

Conversation

roshkhatri
Copy link
Member

This PR forks all the redis tests that were running on the docker-library/official-images.

I have added these as the external tests while we're using the bashbrew scripts to build, run, test and push it.

@roshkhatri roshkhatri requested a review from madolson July 26, 2024 22:00
@roshkhatri
Copy link
Member Author

@yosifkit @tianon would you please take a look at this PR?

@roshkhatri
Copy link
Member Author

@inetol @jrwren would you be interested in reviewing this PR?

Copy link
Contributor

@jrwren jrwren left a comment

Choose a reason for hiding this comment

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

LGTM

If I were nit picking I'd say add the missing trailing newlines and use [[ over [ consistently.

Copy link
Contributor

@inetol inetol left a comment

Choose a reason for hiding this comment

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

Being a fork that removed everything except the specific tests for Valkey there are some files that could be deleted like config.sh, even more important is a deprecated Docker flag as --link that limits my ability to run the tests under Podman as well as some ShellCheck errors that would be convenient to fix.

I have opened a PR to address these issues against this branch here.

@tianon
Copy link
Contributor

tianon commented Jul 27, 2024

I'm not sure why you're copying all the tests, especially without changes; the bashbrew GitHub action will run the tests in config.sh in addition to the upstream DOI tests (which you can also reference in your config).

You can also explicitly set GITHUB_REPOSITORY to control the image name (https://github.com/tianon/docker-qemu/blob/72cd4ebd938b1628a5e6d0abbd2ceac1590aef88/.github/workflows/ci.yml#L34).

You might also want to switch from @HEAD to the latest tagged release of bashbrew so you're not alpha testing our changes like we prefer to. 😄

@roshkhatri
Copy link
Member Author

The reason I am doing this is, in future we might be moving away and we might want to modify the tests and add new ones.
I thought that it would have been better to keep the tests and the code in the same repo for maintainability.

WDYT @tianon, does it makes sense?

.github/workflows/ci.yml Outdated Show resolved Hide resolved
roshkhatri and others added 6 commits July 30, 2024 21:04
Signed-off-by: Roshan Khatri <[email protected]>
Signed-off-by: Roshan Khatri <[email protected]>
Signed-off-by: diana esteves <[email protected]>
Signed-off-by: Roshan Khatri <[email protected]>
Signed-off-by: Roshan Khatri <[email protected]>
@roshkhatri roshkhatri force-pushed the Add-more-tests branch 2 times, most recently from e1fb507 to a427d8b Compare July 30, 2024 21:12
Signed-off-by: Ivan Gabaldon <[email protected]>
Signed-off-by: Roshan Khatri <[email protected]>
@roshkhatri roshkhatri merged commit e3ed566 into valkey-io:mainline Aug 1, 2024
8 checks passed
@roshkhatri roshkhatri deleted the Add-more-tests branch September 4, 2024 20:59
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.

6 participants