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: Add missing test case for from_str exercise #368

Merged
merged 2 commits into from May 3, 2020

Conversation

apatniv
Copy link
Contributor

@apatniv apatniv commented Apr 22, 2020

Added the missing test case for "from_str" exercise.

@AbdouSeck
Copy link
Contributor

Hi @apatniv,

Thanks for offering to improve the test cases for the conversions exercises. I believe the issue #358 brought this up. Could you please take a look at my response there and add the recommended changes to this pull request?

I believe incorporating those changes would go hand in hand with this pull request.

Please feel free to reach out if you have any additional questions.

Thank you,

Abdou

@apatniv
Copy link
Contributor Author

apatniv commented Apr 26, 2020

Abdou

Thanks for your response. I will take a look at #358 and add the necessary checks.

@apatniv
Copy link
Contributor Author

apatniv commented Apr 27, 2020

Would you recommend putting all malformed strings into one test case like this

    fn test_with_malformed_strings() {
        let bad_inputs = [
            "Mark",
            ",",
            ",one",
        ];
        
        for val in &bad_inputs {
            let result = Person::try_from(*val);
            assert!(result.is_err());
        }
        
    }

or create a new test function for each of the cases like

test_missing_comma_and_age
test_missing_name

Former is easier to write but may be bad for reading/debugging purpose. Later, is easier to debug at the expense of verbosity as we need to add similar test cases in try_from_into.rs, from_str.rs and from_into.rs.

IMPO, later approach may be easier for beginners.

@AbdouSeck
Copy link
Contributor

Hi @apatniv,

I am much more drawn to the second option for two reasons:

  1. It aligns with the existing test functions and very easily tells the reader which exact combination of name and age is being tested.
  2. If you use an array of strings, whenever one of them fails the test, the remaining are not tested. Furthermore, in a setup like this, the failure message may not be as clear.

I understand that it's going to be quite verbose, so feel free to do whatever sounds better to you and does not consume too much of your precious time.
Anyway, I am happy to keep the exchange going if you have any questions.

Thank you,

Abdou

@apatniv
Copy link
Contributor Author

apatniv commented May 3, 2020

@AbdouSeck Thanks for the feedback. I add all the possible test cases and tried to follow the conventions in individual test cases.

@AbdouSeck
Copy link
Contributor

Hi @apatniv, this looks fantastic! Thank you so much for the work!
@fmoko this should be ready.

Thank you all!

Abdou

@shadows-withal
Copy link
Member

Thanks!

@shadows-withal
Copy link
Member

@all-contributors add @apatniv for code and test

@allcontributors
Copy link
Contributor

@fmoko

I've put up a pull request to add @apatniv! 🎉

ppp3 pushed a commit to ppp3/rustlings that referenced this pull request May 23, 2022
dmoore04 pushed a commit to dmoore04/rustlings that referenced this pull request Sep 11, 2022
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.

None yet

3 participants