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

MONGOID-5274 Add pending test for Issues with #touch method #5204

Merged
merged 10 commits into from May 24, 2022

Conversation

Neilshweky
Copy link
Contributor

No description provided.


after do
Timecop.return
end
Copy link
Contributor

Choose a reason for hiding this comment

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

In my tests PR I removed Timecop, because I wanted to make sure various timestamps are actually synchronized when touching. Timecop (because it synchronizes time) will lead to false positives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain the case specifically where timecop will lead to false positives?

end

context "when touch is false on belongs_to" do
let(:parent_cls) { TouchableSpec::Referenced::Building }
Copy link
Contributor

Choose a reason for hiding this comment

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

For these types of detailed technical tests I think we should move way from "real world" class names (Building, Hospital, etc.) and have the class names be things like EmbedsOne, EmbeddedIn, etc. which describe their actual relationship.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with that is that the naming space gets too cluttered. How many "NoParent" classes are there going to be, you know? It's also not that big of a deal to just look at the class definition.

@p-mongo
Copy link
Contributor

p-mongo commented May 20, 2022

Is there mergeable code in this PR? If yes can you please retitle it to reflect the changes being made.

@Neilshweky Neilshweky changed the title MONGOID-5274 Issues with #touch method MONGOID-5274 Add pending test for Issues with #touch method May 20, 2022
@Neilshweky
Copy link
Contributor Author

Neilshweky commented May 20, 2022

Is there mergeable code in this PR? If yes can you please retitle it to reflect the changes being made.

This was previously being blocked by MONGOID-5275, but I just explicitly specified the class_name on the models so we don't have to worry about that anymore. I have added a pending test... I think this can be merged as is, and then we can decide if we want to fix this later. As the fix for this depends on our decision regarding MONGOID-5016

@Neilshweky
Copy link
Contributor Author

After we merge this we can probably just close MONGOID-5274 as a duplicate of MONGOID-5016

end

after do
Timecop.return
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this return be moved up to the topmost context added in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests in this PR are separated into two contexts. I have added the return to both of them.

end
end

[ "save!".to_sym, :destroy, :touch].each do |meth|
Copy link
Contributor

Choose a reason for hiding this comment

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

:save! should work no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

embeds_many :entrances
embeds_many :floors
embeds_many :entrances, class_name: "TouchableSpec::Embedded::Entrance"
embeds_many :floors, class_name: "TouchableSpec::Embedded::Floor"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed because Mongoid does not resolve class names correctly?

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 believe so

@Neilshweky Neilshweky requested a review from p-mongo May 23, 2022 17:36
@Neilshweky Neilshweky merged commit 0616bb4 into mongodb:master May 24, 2022
p-mongo pushed a commit to johnnyshields/mongoid that referenced this pull request Jun 6, 2022
* master:
  MONGOID-5366 add fix to 7.3.5 release notes
  MONGOID-5363 Document that upsert replaces existing documents (mongodb#5293)
  Fix MONGOID-5366 #upsert misuses #update_one  (mongodb#5294)
  Fix MONGOID-5139 mongoid.yml template socket timeout default is incorrect (mongodb#5296)
  MONGOID-3601 Don't mark embedded objects as persisted when the parent is invalid (mongodb#5297)
  MONGOID-5077 DocumentNotFound message fails to account for shard key (mongodb#5292)
  MONGOID-5360 Warn when defining field types of BSON::Int64/Decimal128 and other unsupported types (mongodb#5291)
  MONGOID-5363 add test that #upsert replaces existing document (mongodb#5289)
  MONGOID-2951 Mark attributes changed when accessed via #read_attribute / subscript operator (mongodb#5284)
  MONGOID-4935 Fix Errors::InvalidField gives the wrong method name in description (mongodb#5287)
  RUBY-2947 MONGOID-5216 Update Mongoid test tooling for driver/MRSS 5.2/6.0 changes + partial work on app tests fixes (mongodb#5132)
  MONGOID-5345: Fix namespace_hierarchy method for anonymous classes (mongodb#5270)
  MONGOID-5274 Add pending test for Issues with #touch method (mongodb#5204)
  MONGOID-5190 Respect persistence options on auto-saved associations (mongodb#5283)
  MONGOID-5079 clear has_many association locally after destroy_all (mongodb#5278)
  MONGOID-5352 assign_attributes is working incorrectly since 7.3.4 (mongodb#5274)
  MONGOID-5072 Document readonly attributes ignoring assignment (mongodb#5282)
  MONGOID-4843 don't populate .changed_attributes setting has_and_belongs_to_many association (mongodb#5276)
  MONGOID-5214 add failing test case (mongodb#5128)
  MONGOID-5319 Permit driver FLE configuration to be specified in Mongoid config file (mongodb#5251)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants