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

Added basic test cases for resume model #52

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added basic test cases for resume model #52

wants to merge 1 commit into from

Conversation

jasonkhoe
Copy link
Contributor

Added some basic test cases for the Resume model. Still needs test cases for file uploads and PDF validation.

Written in response to this Redmine issue: https://hkn.eecs.berkeley.edu/redmine/issues/98

Oh, and I haven't modified the model yet to actually pass these tests, most of which actually fail.

Looking for feedback, this is the first time I've written an Rspec test, so not sure if I'm following best practices here.

end

it "should have a file" do
@resume.should_not be_valid
Copy link
Member

Choose a reason for hiding this comment

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

One guideline for writing good tests is that you shouldn't be testing the same thing in multiple test cases. In this case, you can have a separate test for testing "should not be valid". In fact, you might even just leave out the check for "should_not be_valid" because the other tests imply that the resume isn't valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm kind of confused here. Is the issue that I'm testing should_not be_valid in multiple test cases?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exactly. Ideally, you should be able to tell what a bug is from which tests are failing. The way they're currently written, if a resume isn't valid when created with no parameters, then every one of these tests will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if there's a test that basically says it "should not be valid", what am I supposed to write regarding test cases for the various validations in the model (presence, range, etc.)? I was modeling the test after some of the other model specs here, but they also repeatedly test should_not be_valid.

Sorry about asking so many questions.

Copy link
Member

Choose a reason for hiding this comment

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

Most of our tests are honestly not written very well. You should be able to just remove all the should_not be_valid statements, since the @resume.errors checks already imply not valid.

@richardxia
Copy link
Member

Overall, 👍 on writing tests and 👍 on code style in general. If you're interested in extra reading, I'd recommend this book, written by the creator of RSpec on Cucumber and RSpec: http://pragprog.com/book/achbd/the-rspec-book

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