Fix windows scheduler flaky tests#533
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a new mechanism for specifying a reference time ("fromNow") when creating and evaluating scheduled tasks. This affects task creation, scheduling triggers, and test logic. The trigger boundary fields are refactored to use pointers to time values instead of strings, and test suites are updated to support and normalise this new time reference for consistent results. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Suggested labels
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
creativeprojects
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
schtasks/task.go(14 hunks)schtasks/task_options.go(1 hunks)schtasks/taskscheduler.go(3 hunks)schtasks/taskscheduler_test.go(4 hunks)schtasks/trigger.go(2 hunks)schtasks/trigger_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
schtasks/taskscheduler.go (2)
Learnt from: creativeprojects
PR: #425
File: schedule/handler_windows.go:97-118
Timestamp: 2025-02-04T14:38:07.701Z
Learning: The shell.SplitArguments function in the resticprofile project returns only []string and does not return any error.
Learnt from: creativeprojects
PR: #459
File: schtasks/schtasks.go:29-30
Timestamp: 2025-02-14T22:53:42.689Z
Learning: In the schtasks package, tasksPath is defined as a constant with value \resticprofile backup\ in taskscheduler.go. It's used as a prefix for managing task paths in the Windows Task Scheduler.
schtasks/trigger.go (1)
Learnt from: creativeprojects
PR: #459
File: schtasks/schtasks.go:29-30
Timestamp: 2025-02-14T22:53:42.689Z
Learning: In the schtasks package, tasksPath is defined as a constant with value \resticprofile backup\ in taskscheduler.go. It's used as a prefix for managing task paths in the Windows Task Scheduler.
schtasks/taskscheduler_test.go (1)
Learnt from: creativeprojects
PR: #459
File: schtasks/schtasks.go:29-30
Timestamp: 2025-02-14T22:53:42.689Z
Learning: In the schtasks package, tasksPath is defined as a constant with value \resticprofile backup\ in taskscheduler.go. It's used as a prefix for managing task paths in the Windows Task Scheduler.
schtasks/trigger_test.go (1)
Learnt from: creativeprojects
PR: #459
File: schtasks/schtasks.go:29-30
Timestamp: 2025-02-14T22:53:42.689Z
Learning: In the schtasks package, tasksPath is defined as a constant with value \resticprofile backup\ in taskscheduler.go. It's used as a prefix for managing task paths in the Windows Task Scheduler.
schtasks/task.go (1)
Learnt from: creativeprojects
PR: #425
File: schedule/calendar_interval.go:98-114
Timestamp: 2025-02-04T14:10:09.439Z
Learning: In schedule/calendar_interval.go, the map inside CalendarInterval is always initialized through newCalendarInterval() or clone(), so additional nil checks on the map are not needed in setCalendarIntervalValueFromType().
schtasks/task_options.go (1)
Learnt from: creativeprojects
PR: #459
File: schtasks/schtasks.go:29-30
Timestamp: 2025-02-14T22:53:42.689Z
Learning: In the schtasks package, tasksPath is defined as a constant with value \resticprofile backup\ in taskscheduler.go. It's used as a prefix for managing task paths in the Windows Task Scheduler.
🔇 Additional comments (18)
schtasks/trigger.go (1)
6-6: Type safety improvement with time package import.Good addition to support the typed time boundaries instead of string representations.
schtasks/trigger_test.go (2)
40-40: Consistent integration with new reference time parameter.All test fixtures properly initialise the new
fromfield totime.Time{}, which maintains existing test behaviour whilst supporting the new reference time functionality.Also applies to: 47-47, 55-55, 62-62, 69-69, 76-76, 84-84, 91-91, 99-99, 106-106, 113-113, 120-120, 128-128, 135-135, 142-142, 150-150, 157-157, 165-165, 172-172, 180-180, 187-187, 195-195
221-221: Function call updated to match new signature.The call to
createTaskDefinitioncorrectly passes the newfromparameter, maintaining consistency with the updated function signature.schtasks/taskscheduler.go (3)
29-29: Required import for new time-based functionality.Addition of the
timepackage import supports the new reference time parameter.
170-175: Well-designed options pattern implementation.The conditional creation of
TaskOptionslice and use ofWithFromNowoption whenfromis non-zero is elegant. This approach:
- Avoids unnecessary allocations when no options are needed
- Provides extensibility for future options
- Maintains clean separation of concerns
55-55: Maintains backwards compatibility.Passing
time.Time{}tocreateTaskDefinitionensures existing behaviour is preserved whilst supporting the new reference time functionality.schtasks/task_options.go (1)
1-22: Clean implementation of the options pattern.This implementation follows Go idioms excellently:
- Minimal, focused interface with single responsibility
- Constructor function
WithFromNowfollows Go naming conventions- Options pattern provides extensibility for future task configuration
- Clean integration with the
Task.setFromNowmethodThe pattern allows for future expansion whilst maintaining a clean API.
schtasks/task.go (5)
33-33: Proper field design for reference time.The
fromNowfield is correctly:
- Unexported to maintain encapsulation
- Excluded from XML serialisation with
xml:"-"- Used as a reference time for scheduling calculations
36-80: Well-structured constructor with options pattern.The
NewTaskconstructor implementation is excellent:
- Maintains backwards compatibility by initialising
fromNowto current time- Properly applies options after base initialization
- Follows the established options pattern consistently
The integration of the options pattern is clean and extensible.
114-117: Consistent state management in setFromNow.The
setFromNowmethod properly maintains consistency by updating both thefromNowfield and theRegistrationInfo.Date, ensuring the task's metadata remains synchronised with its reference time.
121-121: Consistent use of reference time across all trigger methods.All trigger creation methods (
addTimeTrigger,addDailyTrigger,addWeeklyTrigger,addMonthlyTrigger) consistently uset.fromNowinstead oftime.Now(). This ensures:
- Predictable scheduling behaviour
- Better testability with controlled reference times
- Consistent trigger calculations across the system
Also applies to: 139-139, 149-149, 163-163, 182-182, 191-191, 201-201, 216-216, 236-236, 246-246, 266-266, 276-276
121-121: Proper type alignment with trigger structure changes.The
StartBoundaryassignments now use*time.Timepointers, correctly aligning with the type changes intrigger.go. This maintains type consistency across the scheduling system.Also applies to: 149-149, 163-163, 182-182, 201-201, 216-216, 236-236, 266-266, 276-276
schtasks/taskscheduler_test.go (6)
128-166: Well-designed test fixture enhancements for time-based scheduling.The addition of the
fromNowfield provides excellent test coverage for schedule evaluation from different reference points. The specific test cases for "every minute at 12" with reference times before (11:20) and after (13:20) noon effectively test boundary conditions. The commented-out test case is appropriately excluded as generating 60 triggers would be excessive for testing purposes.
171-252: Appropriate use of zero time values for backward compatibility.Using
time.Time{}for the majority of fixtures maintains existing test behaviour whilst enabling the new time reference functionality where specifically needed.
283-283: Correct parameter addition to support time reference functionality.The updated function call properly passes the
fromNowvalue from the test fixture tocreateTaskDefinition.
290-290: Useful diagnostic logging for trigger counts.The logging statement provides valuable insight into the number of triggers generated, which is particularly helpful for debugging complex scheduling patterns.
309-311: Proper task normalisation for consistent comparison.The approach of zeroing the
fromNowfield and converting trigger boundaries to UTC ensures reliable task comparison regardless of timezone differences. This addresses the Windows Task Scheduler's use of local time in XML definitions.
321-337: Well-implemented UTC conversion function with proper safety checks.The
taskInUTCfunction correctly handles timezone normalisation with appropriate nil pointer checks and clear documentation. The implementation properly addresses bothTimeTriggerandCalendarTriggertypes, ensuring consistent test results across different system timezones.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #533 +/- ##
==========================================
+ Coverage 81.09% 81.12% +0.02%
==========================================
Files 137 138 +1
Lines 11071 11083 +12
==========================================
+ Hits 8978 8990 +12
Misses 1668 1668
Partials 425 425
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
creativeprojects
force-pushed
the
fix-schtasks-flaky-tests
branch
from
c8dc86c to
fd5a52f
Compare
|
creativeprojects
force-pushed
the
fix-schtasks-flaky-tests
branch
from
16944f3 to
729153b
Compare
There was a problem hiding this comment.
Pull request overview
Adjusts Windows Task Scheduler task/trigger generation and related tests to reduce time-dependent and timezone-related flakiness when producing and round-tripping task XML.
Changes:
- Switched trigger boundary fields from formatted strings to
*time.Timefor XML marshalling/unmarshalling. - Added an internal
fromNow/WithFromNowoption so tests can deterministically compute “next occurrence” times. - Updated unit/integration tests to tolerate timezone differences and normalize times when comparing exported/imported tasks.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
schtasks/trigger.go |
Change trigger boundary fields to *time.Time to improve timezone handling in XML. |
schtasks/task.go |
Add fromNow support and update trigger creation to store boundaries as *time.Time. |
schtasks/task_options.go |
Introduce Windows-only TaskOption + WithFromNow to set deterministic “now”. |
schtasks/taskscheduler.go |
Thread fromNow into task creation (defaulting to current behavior when zero). |
schtasks/trigger_test.go |
Update regex expectations for timezone, add deterministic “from” control. |
schtasks/taskscheduler_test.go |
Add deterministic “fromNow” fixtures and normalize times to UTC before equality checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.