256 track all blank changes #257

BrianLMatthews · 2024-08-06T16:58:57Z

Add the track_blank_changes option to track changes previously not tracked because both the original and modified values respond with true to blank?

Changes include adding the option, using the option when looping over changes, tests to test the above, documenting the option in README.md, and including the change in CHANGELOG.md.

This is mostly a "get my feet wet" commit, but it adds some tests for the track_blank_changes option being present and having the right default. As I haven't added track_blank_changes as an option yet, these tests fail as expected. I did a rake with out-of-the-box code and saved the results in testresults/0-baseline.txt, after running them through this script: #!/usr/bin/env ruby until (line = gets).nil? line.gsub!(/_id: [[:xdigit:]]{24}/, '_id: <id>/') line.gsub!(/BSON::ObjectId$'[[:xdigit:]]{24}'$/, '<id>') line.gsub!(/[\d.]+/, 'n') if line.start_with?('Finished in ') puts line end to get rid of _ids and the total run time, both of which change every run. I then ran rake after the changes in this commit and saved the output to testresults/1-track_blank_changes-default-tests.txt. I won't include the entire 197 line diff here, but the important part is: $ diff -u testresults/0-baseline.txt testresults/1-track_blank_changes-default-tests.txt | head --- testresults/0-baseline.txt 2024-08-05 11:17:55.000000000 -0700 +++ testresults/1-track_blank_changes-default-tests.txt 2024-08-05 11:21:01.000000000 -0700 [...] -432 examples, 0 failures, 2 pending +432 examples, 6 failures, 2 pending [...] Those 6 failures are all to be expected given that the track_blank_changes_option doesn't exist yet. There were no rubocop complaints.

Originally there was a single test for all blank changes. This commit adds a number more. Now it tests: - Both [nil, <blank thing>] and [<blank thing>, nil] changes. - <blank thing> was originally just an empty Array. Now it can be: - [] - false - '' - A non-empty String consisting of all whitespace characters - {} - It tests with the track_blank_changes option the default, explicitly false, and explicitly true. The tests with track_blank_changes the default or explicitly false succeed, accidentally for now, as there is no such option and the code acts as if track_blank_changes is false, so tests that rely on that succeed. The tests that set track_blank_changes to true fail, as there's no such option and setting it has no effect on the operation of the code. Here's a diff of the test results with just the important parts, not all 422 lines: $ diff -u testresults/1-track_blank_changes-default-tests.txt testresults/2-addition-all-blank-changes.txt --- testresults/1-track_blank_changes-default-tests.txt 2024-08-05 11:21:01.000000000 -0700 +++ testresults/2-addition-all-blank-changes.txt 2024-08-05 11:34:14.000000000 -0700 -432 examples, 6 failures, 2 pending +455 examples, 14 failures, 2 pending The 6 failures are those introduced by commit 52cc180. The 8 additional failures are introduced in this commit. And, just to verify, I commented out the unless clause on lib/mongoid/history/attributes/update.rb:29, which makes track_blank_changes effectively always true, and the tests that don't set it or set it to false fail (as it's always implicitly true with the commented out unless), and the tests that set it to true succeed (the option isn't being set, there is no option, but the code acts as if it's true), all as I'd expect things to work.

There are a number of tests that setting an option value actually sets the option to that value, so this commit adds one for track_blank_changes. Unfortunately I added it before discovering that the whole section isn't testing anything other than that setting a value in a Hash and retrieving that value gives the same value back (and that two of tests even do that wrong). The two tests that are wrong are those for :track_create and :track_destroy. Because they're setting the options to the default value, it's not possible to tell if the tests pass because the values are actually being set or because they're not and just the default value is being used. More problematic is that adding a test like: describe ':any_name' do let(:options) { { any_name: 'any_value' } } it { expect(subject[:any_name]).to be 'any_value' } end passes, as long as any_name is something other than a few special option names (like on). None of the tests between lines 319 and 359 are really testing anything useful IMO. Nevertheless, I added the :track_blank_changes test. Here's the diff of the test output vs that generated after commit 8081c34: $ diff -u testresults/2-additional-all-blank-changes.txt testresults/3-set-track_blank_changes.txt --- testresults/2-additional-all-blank-changes.txt 2024-08-05 11:34:14.000000000 -0700 +++ testresults/3-set-track_blank_changes.txt 2024-08-05 15:46:06.000000000 -0700 @@ -591,6 +591,8 @@ is expected to equal false :track_destroy is expected to equal true + :track_blank_changes + is expected to equal true #remove_reserved_fields is expected to eq ["foo"] is expected to eq [] @@ -1076,7 +1078,7 @@ # /Users/blm/.rvm/gems/ruby-2.3.7/gems/rspec-core-3.13.0/exe/rspec:4:in `<main>' Finished in n minute n seconds (files took n seconds to load) -455 examples, 14 failures, 2 pending +456 examples, 14 failures, 2 pending Failed examples:

Time to start fixing some test failures instead of creating them. Add track_blank_changes as a default option, defaulting to false. $ diff -u testresults/3-set-track_blank_changes.txt testresults/4-add-track_blank_changes-option.txt --- testresults/3-set-track_blank_changes.txt 2024-08-05 15:46:06.000000000 -0700 +++ testresults/4-add-track_blank_changes-option.txt 2024-08-05 19:34:48.000000000 -0700 @@ -504,7 +504,7 @@ [...] -456 examples, 14 failures, 2 pending +456 examples, 8 failures, 2 pending [...] The failures fixed are exactly the 6 introduced in commit 52cc180.

Use the track_blank_changes option. If it's true, any change reported by the change method is tracked, even if both the original and new values are blank (i.e. respond with true to blank?). The tests now all run successfully (and there are no rubocopy complaints, although I did increase the maximum class size in commit d3275e6 as Mongoid::History::Options was exactly at the class size limit). Here are the complete differences between the initial test output and now: $ diff -u testresults/0-baseline.txt testresults/5-use-track_blank_changes-option.txt --- testresults/0-baseline.txt 2024-08-05 11:17:55.000000000 -0700 +++ testresults/5-use-track_blank_changes-option.txt 2024-08-05 20:00:33.000000000 -0700 @@ -320,8 +320,62 @@ should save audit history under relation alias when original and modified value same is expected not to include "emb_ones" - when original and modified values blank - is expected not to include "other_dummy_parent_ids" + when original value blank and modified value nil + when track_blank_changes default + many-to-many field + changes should not include other_dummy_parent_ids + boolean field + changes should not include boolean + empty string field + changes should not include string + all whitespace string field + changes should not include string + when track_blank_changes false + many-to-many field + changes should not include other_dummy_parent_ids + boolean field + changes should not include boolean + empty string field + changes should not include string + all whitespace string field + changes should not include string + when track_blank_changes true + many-to-many field + changes should include other_dummy_parent_ids + boolean field + changes should include boolean + empty string field + changes should include string + all whitespace string field + changes should include string + when original value nil and modified value blank + when track_blank_changes default + many-to-many field + changes should not include other_dummy_parent_ids + boolean field + changes should not include boolean + empty string field + changes should not include string + all whitespace string field + changes should not include string + when track_blank_changes false + many-to-many field + changes should not include other_dummy_parent_ids + boolean field + changes should not include boolean + empty string field + changes should not include string + all whitespace string field + changes should not include string + when track_blank_changes true + many-to-many field + changes should include other_dummy_parent_ids + boolean field + changes should include boolean + empty string field + changes should include string + all whitespace string field + changes should include string Mongoid::History::Options :if @@ -537,6 +591,8 @@ is expected to equal false :track_destroy is expected to equal true + :track_blank_changes + is expected to equal true #remove_reserved_fields is expected to eq ["foo"] is expected to eq [] @@ -824,6 +880,6 @@ # ./spec/unit/attributes/create_spec.rb:340 Finished in n minute n seconds (files took n seconds to load) -432 examples, 0 failures, 2 pending +456 examples, 0 failures, 2 pending [Coveralls] Outside the CI environment, not sending data. 24 new tests, including testing when track_blank_changes is true, and still 0 failures.

BrianLMatthews · 2024-08-06T17:03:27Z

Something seems to have gone awry when updating CHANGELOG.md the second time, the changes to README.md appear in that commit. All the changes are there, just not as organized as I had it for some reason.

dblock

Overall LGTM! Minor asks below. @johnnyshields should take a look.

CI needs fixing, not sure what's up, been a while.

CHANGELOG.md

BrianLMatthews · 2024-08-06T18:19:01Z

CI needs fixing, not sure what's up, been a while.

It looks like it's broken with the original code too. The version of coveralls it grabs uses Hash#slice, which was introduced in ruby 2.5. If I monkey patch that, then it complains about Fixnum#clamp, which was introduced in ruby 2.4 or 2.7 (both did something with clamp, I didn't read far enough to know which did what). If I monkey patch both:

$ diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 935728f..c3d1094 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -1,3 +1,19 @@
+# rubocop:disable all
+class Hash
+  def slice(*keys)
+    keys.each_with_object({}) { |k, hash| hash[k] = self[k] if key?(k) }
+  end
+end
+
+class Fixnum
+  def clamp(p1, p2)
+    return p1 if self.<=> p1 < 0
+    return p2 if self.<=> p2 > 0
+    self
+  end
+end
+# rubocop:enable all
+
 require 'coveralls'
 Coveralls.wear!

then bundle exec rake runs fine:

$ bundle exec rake
[...]
456 examples, 0 failures, 2 pending

So it looks like the gem itself works fine back to at least ruby 2.3.7 (the version I just tried it with), but the specs need 2.5. My preference would be to keep the gem running on older versions and fix the specs.

dblock · 2024-08-07T00:43:42Z

My preference would be to keep the gem running on older versions and fix the specs.

Whatever gets us to green fastest!

BrianLMatthews · 2024-08-07T03:04:53Z

Is the coveralls gem only used for the CI stuff? I uninstalled it, commented out the two lines that reference it in spec/spec_helper.rb, and bundle exec rake runs just fine and produces the same results. So if coveralls isn’t needed that would be the easiest fix. Otherwise the monkey patches in spec/spec_helper.rb (maybe with a version check around them) would be next easiest.

Also, I realized I didn’t bump the version in lib/mongoid/history/version.rb. Do you want me to do that or do you do that when you release?

dblock · 2024-08-07T20:42:53Z

Coveralls is used to report test coverage in CI. It used to work, so something has broken since and needs to be fixed.

Yes, please increment the version in this PR since this is a new feature. We do the increment after the release so that we don't confuse code that has shipped vs. not.

After exploring the failures locally, it appears that term-ansicolor is the culprit. Version 1.10.0 introduced an undocumented (as far as I can tell, but its CHANGES file stops at 1.7.1 :-( ) dependency on ruby 2.5, by using Hash#slice. So use 1.9.0 you say! Except 1.9.0 doesn't work at all (it's trying to call start_with? on a Symbol). I tried all the commits between 1.9.0 and 1.10.0 and all had some problem or another. Luckily, the latest version needed is 1.3.x (by coveralls), so if I specify that here, that's the one that's installed and bundle rake exec runs fine, so hopefully this will fix the CI failures. We'll see...

BrianLMatthews · 2024-08-08T20:33:18Z

Ok, bumped the internal version number, then made a change to Gemfile that I think, maybe, fingers crossed, will fix the CI problem. If not I did it in its own commit so it can easily be reverted.

dblock

Minor ask for CHANGELOG.

CHANGELOG.md

dblock

Sorry if I wasn't clear about CHANGELOG.

@johnnyshields you're good with this change?

CHANGELOG.md

dblock

Thanks for hanging in here with me @BrianLMatthews. Will merge if I don't hear from @johnnyshields soon :)

johnnyshields · 2024-08-22T16:52:20Z

Its cool! Please go ahead and merge. Thanks!

dblock · 2024-08-23T17:08:59Z

I'm merging this, thank you!

We have some jruby and 2.7 test failures that look unrelated, @BrianLMatthews maybe you could take a look and add Ruby 3.1/2/3?

BrianLMatthews · 2024-08-25T17:31:43Z

Thanks for merging this! We're working on upgrading our app to Ruby 3.something, when that's done I may be able to take a look. Don't let that stop someone else from looking at it though, don't know when that will be.

Thanks again!

BrianLMatthews added 6 commits August 5, 2024 11:24

Update CHANGLOG

15a8f10

BrianLMatthews force-pushed the 256-track_all_blank_changes branch from 1f47693 to 15a8f10 Compare August 6, 2024 17:01

BrianLMatthews mentioned this pull request Aug 6, 2024

Change from non-existent to false not being tracked #256

Open

dblock requested changes Aug 6, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update version to 0.9.0 per request

b3f25e8

BrianLMatthews added 2 commits August 8, 2024 12:27

Update the internal version number as well

1f055bb

dblock requested changes Aug 18, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update per request

95e0536

dblock requested changes Aug 19, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Show resolved Hide resolved

Update per request

a1e113f

dblock approved these changes Aug 22, 2024

View reviewed changes

dblock merged commit c8c4de1 into mongoid:master Aug 23, 2024
12 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

256 track all blank changes #257

256 track all blank changes #257

BrianLMatthews commented Aug 6, 2024 •

edited

Loading

BrianLMatthews commented Aug 6, 2024

dblock left a comment •

edited

Loading

BrianLMatthews commented Aug 6, 2024 •

edited

Loading

dblock commented Aug 7, 2024

BrianLMatthews commented Aug 7, 2024 •

edited

Loading

dblock commented Aug 7, 2024

BrianLMatthews commented Aug 8, 2024

dblock left a comment

dblock left a comment

dblock left a comment

johnnyshields commented Aug 22, 2024 via email

dblock commented Aug 23, 2024

BrianLMatthews commented Aug 25, 2024

256 track all blank changes #257

256 track all blank changes #257

Conversation

BrianLMatthews commented Aug 6, 2024 • edited Loading

BrianLMatthews commented Aug 6, 2024

dblock left a comment • edited Loading

Choose a reason for hiding this comment

BrianLMatthews commented Aug 6, 2024 • edited Loading

dblock commented Aug 7, 2024

BrianLMatthews commented Aug 7, 2024 • edited Loading

dblock commented Aug 7, 2024

BrianLMatthews commented Aug 8, 2024

dblock left a comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

johnnyshields commented Aug 22, 2024 via email

dblock commented Aug 23, 2024

BrianLMatthews commented Aug 25, 2024

BrianLMatthews commented Aug 6, 2024 •

edited

Loading

dblock left a comment •

edited

Loading

BrianLMatthews commented Aug 6, 2024 •

edited

Loading

BrianLMatthews commented Aug 7, 2024 •

edited

Loading