-
Notifications
You must be signed in to change notification settings - Fork 4k
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
credit_spec
tests for #credit_count
are ineffective and do not fail when expected to
#20653
Comments
Thanks for the issue, we will take it into consideration! Our team of engineers is busy working on many types of features, please give us time to get back to you. To our amazing contributors: issues labeled If this is a feature request from an external contributor (not core team at Forem), please close the issue and re-post via GitHub Discussions. To claim an issue to work on, please leave a comment. If you've claimed the issue and need help, please ping @forem-team. The OSS Community Manager or the engineers on OSS rotation will follow up. For full info on how to contribute, please check out our contributors guide. |
Fixes forem#20653 Credit uses counter culture to create and populate columns. The code currently populates these columns explicitly. However, not populating the credits_count columns should cause tests to fail. It doesn't. The credits test subjects are set up with: ``` let(:user_credits) { create_list(:credit, 2, user: user) } let(:org_credits) { create_list(:credit, 1, organization: organization) } ``` However, these are not tested. They do not point to the correct `user` or `organisation`. Using `let!`, eager loads these associations and creates the necessary credits, and references the lazily loaded `user` and `organization` subjects.
Fixes forem#20653 Credit uses counter culture to create and populate columns. The code currently populates these columns explicitly. However, not populating the credits_count columns should cause tests to fail. It doesn't. The credits test subjects are set up with: ``` let(:user_credits) { create_list(:credit, 2, user: user) } let(:org_credits) { create_list(:credit, 1, organization: organization) } ``` However, these are not tested. They do not point to the correct `user` or `organisation`. Using `let!`, eager loads these associations and creates the necessary credits, and references the lazily loaded `user` and `organization` subjects.
NB, I've started a discussion (#20671) asking for guidance on how to demonstrate that the tests are currently broken. |
Bug:
credit_spec
tests for#credit_count
are ineffective and do not fail when expected toCredit
uses counter culture to create and populate columns. The code currently populates these columns explicitly. Unpopulatedcredits_count
columns should cause tests to fail.The problem
The
column_count
tests pass regardless of whether the column is populated or not.To Reproduce
Comment out the third item in
column_names:
. (The item that tellscounter_culture
how to populatecredits_count
.)Now the
credits_count
columns ofuser
andorganization
should be empty. Despite this change, the test passes:Remedy
I'm not familiar with RSpec but I believe the problem is becauselet(:user) { create(:user) }
creates the user before it has chance to create the credits:let!(:user_credits) { create_list(:credit, 2, user: user) }
. Therefore, a count of zero credits is compared to the size of an empty array. The same goes fororganization
.To fix this, I've used the eager loading version
let!
to ensure the credits are created with the correct associations.The tests now fail.
After re-instating the commented out line, the tests pass as expected.
Addendum: Are
spent_credits_count
andunspent_credits_count
tests satisfactory?Now, repeating the above for
unspent_credits_count
andspent_credits_count
, I find the test pass regardless.How do I know if this is expected or not?
I break
Credit
by swapping the scopes.Tests fail as expected. Failing tests:
I believe the existing tests are enough to ensure that
spent_credits_count
andunspent_credits_count
are populated correctly and that there is no need to populate these columns explicitly. (Here the counter culture docs give an example of creating columns without the:column_names
argument.)To be doubly sure, jumping into the tests with pry, I can see that everything seems to be as expected and the tests should pass.
I will raise this refactor in discussions.
The text was updated successfully, but these errors were encountered: