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.
| type TimeTrigger struct { | ||
| Enabled *bool `xml:"Enabled"` // indicates whether the trigger is enabled | ||
| StartBoundary string `xml:"StartBoundary"` | ||
| StartBoundary *time.Time `xml:"StartBoundary"` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify XML serialisation behaviour for time.Time pointers.
The change from string to *time.Time for StartBoundary and EndBoundary fields improves type safety significantly. However, ensure that time.Time XML marshalling produces the expected format for the Windows Task Scheduler.
Also applies to: 24-25
🏁 Script executed:
#!/bin/bash
# Verify how time.Time is serialised to XML in Go
cd /tmp
cat > test_time_xml.go << 'EOF'
package main
import (
"encoding/xml"
"fmt"
"time"
)
type TestStruct struct {
TimeField *time.Time `xml:"TimeField"`
}
func main() {
now := time.Now()
test := TestStruct{TimeField: &now}
data, err := xml.MarshalIndent(test, "", " ")
if err != nil {
fmt.Printf("Error: %v\n", err)
return
}
fmt.Printf("XML output:\n%s\n", data)
// Test with nil pointer
testNil := TestStruct{TimeField: nil}
dataNil, err := xml.MarshalIndent(testNil, "", " ")
if err != nil {
fmt.Printf("Error with nil: %v\n", err)
return
}
fmt.Printf("\nXML output with nil:\n%s\n", dataNil)
}
EOF
go run test_time_xml.goLength of output: 285
Ensure Windows Task Scheduler–compatible XML for time.Time pointers
Our test shows Go’s default XML marshalling of *time.Time emits RFC3339Nano (e.g. 2025-07-27T17:38:11.303820919Z) and omits nil pointers entirely:
<StartBoundary>2025-07-27T17:38:11.303820919Z</StartBoundary>Windows Task Scheduler requires timestamps in the form “YYYY-MM-DDThh:mm:ss” (no fractional seconds or timezone). To guarantee compatibility, wrap *time.Time in a custom type that implements xml.Marshaler (and optionally xml.Unmarshaler) using t.Format("2006-01-02T15:04:05"). For example:
// In schtasks/trigger.go
// Replace the field types:
type Trigger struct {
StartBoundary *TaskTime `xml:"StartBoundary"`
EndBoundary *TaskTime `xml:"EndBoundary"`
// …
}
// TaskTime ensures formatting matches Task Scheduler’s schema.
type TaskTime time.Time
func (t *TaskTime) MarshalXML(e *xml.Encoder, start xml.StartElement) error {
if t == nil {
return nil
}
formatted := (*time.Time)(t).Format("2006-01-02T15:04:05")
return e.EncodeElement(formatted, start)
}
func (t *TaskTime) UnmarshalXML(d *xml.Decoder, start xml.StartElement) error {
var v string
if err := d.DecodeElement(&v, &start); err != nil {
return err
}
tt, err := time.Parse("2006-01-02T15:04:05", v)
if err != nil {
return err
}
*t = TaskTime(tt)
return nil
}– Apply this pattern to both StartBoundary and EndBoundary in schtasks/trigger.go to ensure correct formatting.
🤖 Prompt for AI Agents
In schtasks/trigger.go at line 18, the StartBoundary field uses *time.Time which
marshals to XML with fractional seconds and timezone, incompatible with Windows
Task Scheduler. To fix this, define a custom TaskTime type wrapping time.Time
that implements xml.Marshaler and xml.Unmarshaler, formatting time as
"2006-01-02T15:04:05" without fractional seconds or timezone. Replace
StartBoundary and EndBoundary fields to use *TaskTime instead of *time.Time, and
implement the MarshalXML and UnmarshalXML methods on TaskTime to handle the
custom formatting.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #533 +/- ##
==========================================
+ Coverage 79.69% 79.72% +0.02%
==========================================
Files 137 138 +1
Lines 13467 13483 +16
==========================================
+ Hits 10732 10748 +16
Misses 2310 2310
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. 🚀 New features to boost your workflow:
|
|
creativeprojects
force-pushed
the
fix-schtasks-flaky-tests
branch
from
c8dc86c to
fd5a52f
Compare
|
1 participant
No description provided.