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

Simplify the integration test code #376

Merged
merged 5 commits into from
Nov 4, 2024
Merged

Conversation

iddm
Copy link
Collaborator

@iddm iddm commented Nov 24, 2023

No description provided.

@iddm iddm requested a review from MeirShpilraien November 24, 2023 10:16
@iddm iddm self-assigned this Nov 24, 2023
connection: Connection,
}

static mut TEST_PORT: AtomicU16 = AtomicU16::new(6479);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let use lazy static and avoid the unsafe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems that it isn't relevant anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am removing the mut and the unsafe, so this remains a piece of code without lazy_static.

Copy link
Collaborator

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

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

👍
One small comment about avoiding the unsafe block.

@iddm
Copy link
Collaborator Author

iddm commented Apr 29, 2024

@MeirShpilraien, would you please review once again?

@ashtul
Copy link

ashtul commented May 26, 2024

@iddm @MeirShpilraien
Why not move the change into utils.rs and expose it as part of the crate?
It would allow users to use the code to test their modules.

@iddm
Copy link
Collaborator Author

iddm commented May 27, 2024

@iddm @MeirShpilraien Why not move the change into utils.rs and expose it as part of the crate? It would allow users to use the code to test their modules.

We need it just in one single file, there is no need to create a file of 20 lines of code to just fully import it later from within another file. Doesn't really contribute to the cleanliness of the code here. If at least we had two files for that, - sure.

MeirShpilraien
MeirShpilraien previously approved these changes May 27, 2024
@MeirShpilraien
Copy link
Collaborator

We need it just in one single file, there is no need to create a file of 20 lines of code to just fully import it later from within another file. Doesn't really contribute to the cleanliness of the code here. If at least we had two files for that, - sure.

I agree but also I think that its a nice idea that users will be able to test their module with rust and run it with cargo test. I believe that if we want to do it, we should make a crate that will expose all the needed functionality (setting up Redis server with the module and with/without replication or on cluster mode, aof enabled/disabled, and probably there are much more things that we want to do). That said, all this functionalities are already exists on RLTest so maybe its not worth it?

@iddm
Copy link
Collaborator Author

iddm commented May 27, 2024

We need it just in one single file, there is no need to create a file of 20 lines of code to just fully import it later from within another file. Doesn't really contribute to the cleanliness of the code here. If at least we had two files for that, - sure.

I agree but also I think that its a nice idea that users will be able to test their module with rust and run it with cargo test. I believe that if we want to do it, we should make a crate that will expose all the needed functionality (setting up Redis server with the module and with/without replication or on cluster mode, aof enabled/disabled, and probably there are much more things that we want to do). That said, all this functionalities are already exists on RLTest so maybe its not worth it?

This isn't something specific to Redis, redismodule (this crate) or redis modules, or Rust in general. Anyone can write their own tests. But, I had already forgotten we had the utils.rs file there. Now that I know it is there, it makes sense to move it there. Perhaps, I had already done it this way a long time ago before even this PR, but we closed it for another reason, cuz now I remember moving the things into it. So, yes, I'll move it to the utils.rs.

@iddm
Copy link
Collaborator Author

iddm commented Nov 4, 2024

@MeirShpilraien can you just merge it, please?

let config_get = |config: &str| -> Result<String> {
let mut con =
get_redis_connection(port).with_context(|| "failed to connect to redis server")?;
let config_get = |con: &mut TestConnection, config: &str| -> Result<String> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder why the closure itself can not borrow the con here (as it was before)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think because it is mutable and would require double mutable borrow, which the borrow checker won't like.

@iddm iddm merged commit 08281fe into master Nov 4, 2024
1 of 2 checks passed
@iddm iddm deleted the simplify-the-integration-test-code branch November 4, 2024 12:17
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.

3 participants