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

Test fixes #12

Merged
merged 1 commit into from
Aug 25, 2021
Merged

Test fixes #12

merged 1 commit into from
Aug 25, 2021

Conversation

TornaxO7
Copy link
Owner

Adjusted all tests so all are passing now.

Question

While I was refactoring the tests, I encountered often pretty duplicated code like in the doc example of config/model.rs for the new_with_envelope method... I was thinking to create a new trait, which is only active during tests which you can use to create instances of a struct with real-case values, which makes it different compared to the Default trait.

In summary:
The Test trait should make it possible, to create new instances of a struct with near real-case attributes for testing while the Default trait can include "empty" attributes (like an empty vector or None and so on). What do you think?

Adjusted all tests so all are passing now.
@TornaxO7 TornaxO7 added this to the Msg refactoring milestone Aug 24, 2021
@TornaxO7 TornaxO7 requested a review from soywod August 24, 2021 22:20
@TornaxO7
Copy link
Owner Author

@soywod review pls

@TornaxO7
Copy link
Owner Author

@soywod could you also please answer my suggestion with the Test trait?

@TornaxO7 TornaxO7 merged commit 04d1c76 into mail_refactor Aug 25, 2021
@TornaxO7 TornaxO7 deleted the test_fixer branch August 25, 2021 10:35
@soywod
Copy link
Collaborator

soywod commented Aug 25, 2021

Mmh I'm not so fan of having testing code outside of tests modules. It's not sth I've seen in other libs. I think we should not mix up codes. A dedicated function or a macro inside tests modules sounds more straightforward to me (and a better solution).

@TornaxO7
Copy link
Owner Author

Mmh I'm not so fan of having testing code outside of tests modules.

Hold on, you're missunderstanding something.
I meant something like this:
lib.rs:

#[cfg(tests)]
pub trait Test {
    fn get_test() -> Self;
}

and then you can do the following for example for msg/model/msg.rs:

#[cfg(tests)]
impl Test for Msg {
    fn test() -> Self {
       // normal stuff
    }
}```

    }

@soywod
Copy link
Collaborator

soywod commented Aug 26, 2021

What are the benefits of such a trait? I mean, we could just write tests directly inside the test module, why bothering with implementing a trait? Sorry if I don't understand well what you mean.

@TornaxO7
Copy link
Owner Author

TornaxO7 commented Aug 26, 2021

Because I think that it would safe us some duplicated code. Let's say you need a Msg struct for a test in the imap-connection directory and another instance in the body.rs file. You'd create in both places [a similiar]/[an equal] function for both test modules (I think) or do you have a better idea? Because I'd like to avoid cases like here but that's probably my fault.

@soywod
Copy link
Collaborator

soywod commented Aug 26, 2021

The Msg should be tested once at one particular place, we should not need to have multiple instances of Msg. Now, if you really see the use case then do, I'm following you!

@TornaxO7
Copy link
Owner Author

No wait, I don't want to force you!!! I just saw that you also used an account in the test for imap (it says that you don't use it) so I thought that it might suit if we include this trait.

@soywod
Copy link
Collaborator

soywod commented Aug 26, 2021

No no you don't force me, I don't see the need but your are the one refactoring so you should know better, I really trust you! If you think it is needed then you can go, really 😃

@TornaxO7
Copy link
Owner Author

hm... ok, I'll think about it how to fix this...

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.

2 participants