Skip to content

MONGOID-5173 Specs should use bang (!) methods as much as possible #5061

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Jan 21, 2022
Merged

MONGOID-5173 Specs should use bang (!) methods as much as possible #5061

merged 14 commits into from
Jan 21, 2022

Conversation

johnnyshields
Copy link
Contributor

@johnnyshields johnnyshields commented Aug 29, 2021 •
edited
Loading

Bang methods .create!, .save!, and .update_attributes! ensure that the methods actually persisted to the DB and do not have silent failures. They will raise an error (which will make test red) if they fail.

Follow-up from conversation in #5029

@johnnyshields johnnyshields mentioned this pull request Aug 29, 2021
Merged
2 tasks
p-mongo
p-mongo reviewed Aug 29, 2021
View reviewed changes
Copy link
Contributor

@p-mongo p-mongo left a comment

Choose a reason for hiding this comment

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

Hi Johnny,

Did you read (i.e. review) this diff? Green CI doesn't guarantee the tests are correct in that they are testing, for example, the desired functionality.

@johnnyshields johnnyshields changed the title MONGOID-5173 Specs should use bang (!) methods as much as possible Draft: MONGOID-5173 Specs should use bang (!) methods as much as possible Aug 29, 2021
@johnnyshields johnnyshields force-pushed the MONGOID-5173-specs-bang-methods branch from 6c181f3 to 99f0e19 Compare August 29, 2021 15:40
@johnnyshields johnnyshields changed the title Draft: MONGOID-5173 Specs should use bang (!) methods as much as possible MONGOID-5173 Specs should use bang (!) methods as much as possible Aug 29, 2021
@johnnyshields
Copy link
Contributor Author

johnnyshields commented Aug 29, 2021 •
edited
Loading

@p-mongo ready for your review.

In some tests, only one of #create or #create! is tested. For these, I have intentionally changed to #create!, because:

  1. #create! calls #create internally (i.e. greater code coverage) and
  2. it is more likely to cause a failure (i.e. an error) if the model didn't save as the test writer intended.

By the way, in the process of making this PR I did find several places where models were not being saved as intended. The most common was Account missing name field.

Regarding this comment:

Green CI doesn't guarantee the tests are correct in that they are testing

Except in a limited number of specific specs like Savable, Persistable etc. green actually DOES guarantee correctness, because bang (!) calls non-bang internally. (Even in Savable, etc. calling bang instead of non-bang would be acceptable, but I've reverted it anyway.) I'd challenge you to find any example in this PR where the change to bang fundamentally alters the meaning of the test (assuming spec is passing.)

@johnnyshields
Copy link
Contributor Author

@p-mongo this PR is going to continually generate conflicts as new PRs are merged so it would be ideal if we could merge it first.

p and others added 2 commits October 5, 2021 19:24
@p
Merge branch 'master' into pr-5061
6c2319f 
* master:
  Remove remaining encoding declarations (mongodb#5088)
  Add a more descriptive error when spec load fails (mongodb#5066)
  MONGOID-5191 Proper order for after_create callback (mongodb#5087)
  MONGOID-5189 Proper order of *_save callbacks (mongodb#5083)
  MONGOID-5098 Standardize/improve Range queries -- .where(field: 1..3) (mongodb#5025)
@johnnyshields
Merge branch 'master' into MONGOID-5173-specs-bang-methods
e457ab3
@johnnyshields
Copy link
Contributor Author

@p-mongo @comandeo if the specs are passing can we please merge this?

@johnnyshields
Copy link
Contributor Author

@comandeo let's merge this?

@johnnyshields johnnyshields mentioned this pull request Dec 18, 2021
Merged
@p
Merge branch 'master' into pr-5061
1069e27 
* master:
  MONGOID-5173 Specs should use bang (!) methods (without describe/context change) (mongodb#5109)
  Fix doc syntax
  RUBY-2783 Require bson 4-stable for Mongoid as bson master is 5.0 and not compatible with driver (mongodb#5113)
  MONGOID-5208 fix error on reloading nil embedded document (mongodb#5116)
  MONGOID-5206 fix bug where embedded document is not re-embedded (mongodb#5115)
  MONGOID-5207 Add Ruby 3.1 to GH Actions (mongodb#5114)
  MONGOID-5207 Use YAML.safe_load in specs (mongodb#5112)
  MONGOID-5199 Reloadable#reload_embedded_document doesn't respect session (mongodb#5105)
  Fix MONGOID-5198 Calling children_changed? on a deep cyclical data structure will cause semi-infinite looping (mongodb#5104)
p-mongo
p-mongo reviewed Jan 3, 2022
View reviewed changes
Copy link
Contributor

@p-mongo p-mongo left a comment

Choose a reason for hiding this comment

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

The rest of this PR seems to only affect update_attributes. It looks like we currently only have tests for update_attributes and not update_attributes!, so perhaps both should be minimally covered?

spec/mongoid/persistable/savable_spec.rb Outdated
@@ -296,7 +296,7 @@
# no conflicts.
#expect(truck.atomic_updates[:conflicts]).to eq nil

expect { truck.save! }.not_to raise_error
expect(truck.save).to eq true
Copy link
Contributor

Choose a reason for hiding this comment

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

@johnnyshields I am having difficulty understanding the changes in this file, could you explain them please?

Copy link
Contributor Author

@johnnyshields johnnyshields Jan 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

expect { truck.save! }.not_to raise_error

is equivalent to:

expect(truck.save).to eq true

Both statements verify the document saved successfully. The latter syntax more clearly expresses the intention of the spec in this case: we are specifically testing that the document was saved.

@johnnyshields
Copy link
Contributor Author

@p-mongo is my comment here sufficient to merge? Tests are green...

p added 3 commits January 21, 2022 04:25
@p
Merge branch 'master' into pr-5061
aac2245 
* master:
  MONGOID-5098 Bug fix: Range.mongoize should mongoize its members (mongodb#5108)
  MONGOID-5212 Support for Decimal128 type (mongodb#5125)
  MONGOID-5213 Document changes to BigDecimal type and addition of global flag (mongodb#5126)
  MONGOID-5209 Document "Custom Field Options" functionality + various docs improvements (mongodb#5120)
  MONGOID-5193 rails 7 support  (mongodb#5122)
  Fix default topology name - should be standalone (mongodb#5130)
  Update MRSS (mongodb#5129)
  MONGOID-5207 Test Ruby 3.1 on Evergreen (mongodb#5123)
  MONGOID-5186 .with_scope should restore previous scope (mongodb#5127)
  MONGOID-5161 - [Documentation Only] Clarify the policy for working with . and $ named fields which were introduced in MongoDB 5.0 (mongodb#5051)
@p
revert savable changes
a7f3c7a
@p
update_attributes! delegation test
4b264fd
@p-mongo p-mongo requested a review from comandeo January 21, 2022 09:55
@p-mongo p-mongo merged commit e840b10 into mongodb:master Jan 21, 2022
p-mongo pushed a commit that referenced this pull request Mar 11, 2022
@johnnyshields @comandeo-mongo @p
MONGOID-5173 Specs should use bang (!) methods as much as possible (#…
09afa78 
…5061)

* MONGOID-5173 Specs should use bang (!) methods as much as possible

* Update app_spec.rb

* Update shardable_spec.rb

* revert savable changes

* update_attributes! delegation test

Co-authored-by: shields <shields@tablecheck.com>
Co-authored-by: Dmitry Rybakov <dmitry.rybakov@mongodb.com>
Co-authored-by: Oleg Pudeyev <code@olegp.name>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@comandeo comandeo comandeo approved these changes

@p-mongo p-mongo p-mongo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@johnnyshields @comandeo @p-mongo @comandeo-mongo @p