Skip to content

Minor fixes#651

Merged
scothis merged 4 commits intoreconcilerio:mainfrom
alfatraining:0-minor-fixes
Sep 29, 2025
Merged

Minor fixes#651
scothis merged 4 commits intoreconcilerio:mainfrom
alfatraining:0-minor-fixes

Conversation

@annismckenzie
Copy link
Contributor

While upgrading one of our projects to the latest release I came across a few nits in the code and decided to put them into a separate branch and open a PR.

  1. The RetrieveNow method is misspelled across 3 comments so it was probably copy'n'pasted.
  2. In tc.Now == (time.Time{}) linters suggest to use time.Equal instead but what's really tested here is whether an explicit time has been set or not so checking time.Now.IsZero() looks like the prudent thing to do. I couldn't find unit tests for the testing facilities so I added one for this. I can of course also remove this again.

Thank you for your hard work, using this library has started to improve our operators massively.

Signed-off-by: Daniel Lohse <info@asapdesign.de>
…` set

Signed-off-by: Daniel Lohse <info@asapdesign.de>
@codecov
Copy link

codecov bot commented Sep 27, 2025

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 59.48%. Comparing base (ec87995) to head (0d880f9).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
testing/subreconciler.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #651      +/-   ##
==========================================
+ Coverage   57.28%   59.48%   +2.19%     
==========================================
  Files          39       39              
  Lines        4507     4507              
==========================================
+ Hits         2582     2681      +99     
+ Misses       1811     1716      -95     
+ Partials      114      110       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@scothis
Copy link
Member

scothis commented Sep 27, 2025

@annismckenzie thanks for this. The intent behind the Now field is for a user to change the current time used within a reconciler for the specific test case. The proposed test is observing the defaulted value as a side effect. I'll rework the test to set a custom Now as well a assert the default time.

Rather than reading the value of `Now` on `ReconcilerTestCase`, the
field is used to set the time for that test case and the set value is
asserted within the reconcile.

Signed-off-by: Scott Andrews <scott@andrews.me>
@scothis
Copy link
Member

scothis commented Sep 27, 2025

Opened alfatraining#2

@annismckenzie
Copy link
Contributor Author

I'll rebase after #654 was merged if that's alright with you.

@scothis scothis merged commit 7550f72 into reconcilerio:main Sep 29, 2025
3 of 4 checks passed
@annismckenzie annismckenzie deleted the 0-minor-fixes branch September 30, 2025 09:30
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