LG-15493 | Enhance AAMVA maintenance windows spec #11753
Conversation
Previously, an invalid key could break the proofer. Now we test for that, and also ensure they're valid cron expressions. In doing so, I found one that wasn't.
[skip changelog]
| end | ||
|
|
||
| windows.each do |window| | ||
| it "consists of a valid cron expression and numeric duration (#{state})" do |
There was a problem hiding this comment.
While it feels weird, I interpolate state name here so the test is a little easier to follow. The output looks like:
consists of a valid cron expression and numeric duration (IN)
consists of a valid cron expression and numeric duration (TN) (FAILED - 1)
consists of a valid cron expression and numeric duration (MD)
consists of a valid cron expression and numeric duration (VA)
consists of a valid cron expression and numeric duration (DC)
consists of a valid cron expression and numeric duration (MA)
Makes it a lot easier to investigate a failure.
| @@ -161,7 +161,7 @@ class AamvaStateMaintenanceWindow | |||
| ], | |||
| 'UT' => [ | |||
| # 3rd Sunday of every month 1:00 am. to 9:00 am. ET | |||
| { cron: '0 1 0 0 Sun#3', duration_minutes: 8 * 60 }, | |||
| { cron: '0 1 * * Sun#3', duration_minutes: 8 * 60 }, | |||
There was a problem hiding this comment.
🤦♂️ This meant it would run on the 0th day of the 0th month. Our calendar system predates Kernighan and Ritchie, so this was a nonsensical date.
The test failure shown was:
2) Idv::AamvaStateMaintenanceWindow MAINTENANCE_WINDOWS consists of a cron expression and numeric minutes
Failure/Error: expect(cron).to be_a(Fugit::Cron)
expected nil to be a kind of Fugit::Cron
# ./spec/services/idv/aamva_state_maintenance_window_spec.rb:84:in `block (5 levels) in <top (required)>'
# ./spec/services/idv/aamva_state_maintenance_window_spec.rb:77:in `each'
# ./spec/services/idv/aamva_state_maintenance_window_spec.rb:77:in `block (4 levels) in <top (required)>'
Previously, an invalid key could break the proofer. Now we test for that, and also ensure they're valid cron expressions. In doing so, I found one that wasn't.
This way both fields are mandatory.
…nto mattw/better-test-mws
n1zyy
changed the title
| { cron: cron, duration_minutes: window[:duration_minutes] } | ||
| end | ||
| end | ||
| end.freeze |
There was a problem hiding this comment.
This clunkiness was for inline readability. Now that we have a Window Data object, though, I was able to implement a lil' cron_val method that performs this instead of building a whole separate data structure.
Academically, this lacks memoization since it's implemented on a frozen structure, which isn't great. IMHO this is acceptable because we're doing the parse_cron call after filtering down to the (US) state level, so worse case it's only a few calls, which seems to only take a millisecond or so in local testing.
There was a problem hiding this comment.
Very optional, but you might be able to throw a lazy into the windows_for_state return value to get some optimization back, so that we only map up to the first .any? hit.
| ], | ||
| 'CT' => [ | ||
| # Daily, 3:00 am. to 4:00 am. ET. | ||
| { cron: '0 3 * * *', duration_minutes: 1 }, | ||
| Window.new(cron: '0 3 * * *', duration: 1.hour), |
There was a problem hiding this comment.
An unexpected perk of moving to using a Duration is that doing the change semi-manually caused me to catch and fix this 1-minute vs. 1-hour mistake.
| @@ -5,51 +5,57 @@ class AamvaStateMaintenanceWindow | |||
| # All AAMVA maintenance windows are expressed in 'ET' (LG-14028), | |||
| TZ = 'America/New_York' | |||
|
|
|||
| Window = Data.define(:cron, :duration) do | |||
| def cron_val | |||
There was a problem hiding this comment.
Minor: I'd generally prefer we avoid the abbreviation since I don't think it's worth the potential readability hit to save two characters.
| def cron_val | |
| def cron_value |
| def cron_val | ||
| Time.use_zone(TZ) { Fugit.parse_cron(cron) } | ||
| end | ||
| end.freeze |
There was a problem hiding this comment.
I'm unsure, but does .freeze do anything for us here? 🤔
| { cron: cron, duration_minutes: window[:duration_minutes] } | ||
| end | ||
| end | ||
| end.freeze |
There was a problem hiding this comment.
Very optional, but you might be able to throw a lazy into the windows_for_state return value to get some optimization back, so that we only map up to the first .any? hit.
🎫 Ticket
LG-15493
🛠 Summary of changes
A previous commit had
duration:instead ofduration_minutes:in one entry, and no test caught that this was wrong.Now there is one. As a bonus, we also test that the cron expression is valid, and that turned up one failure which I also fixed.