Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a failing test for tinsel not being used in early December #727

Merged
merged 2 commits into from Dec 3, 2019

Conversation

@jennyd
Copy link
Contributor

jennyd commented Dec 2, 2019

2nd December 2019 is a substitute bank holiday in Scotland for St Andrew's Day. The current implementation of Calendar#bunting_styles uses tinsel for all bank holidays with "bunting": true in December and January, but it should probably only do that for the ones for Christmas and New Year:

Screenshot 2019-12-02 at 16 55 16

There were substitute days for St Andrew's Day in 2013 and 2014 as well. I've used one of those in the test because for some reason the test isn't failing today when I use the 2019 date - maybe something to do with Timecop?

2nd December 2019 is a substitute bank holiday in Scotland for St Andrew's
Day. The current implementation of `Calendar#bunting_styles` uses tinsel for
all bank holidays with `"bunting": true` in December and January, but it
should probably only do that for the ones for Christmas and New Year.

There were substitute days for St Andrew's Day in 2013 and 2014 as well. I've
used one of those in the test because for some reason the test isn't failing
today when I use the 2019 date - maybe something to do with Timecop?
@issyl0

This comment has been minimized.

Copy link
Member

issyl0 commented Dec 2, 2019

I pushed the branch to origin so the tests run. Hi, @jennyd!

@issyl0

This comment has been minimized.

Copy link
Member

issyl0 commented Dec 2, 2019

The test fails, as expected because there's currently tinsel when there shouldn't be.

Failure:
BankHolidaysTest#test_: showing bunting on bank holidays should not use tinsel bunting for bank holidays in early December.  [/var/lib/jenkins/workspace/iling-test-tinsel-st-andrews-day/test/integration/bank_holidays_test.rb:201]:
Expected false to be truthy.
@issyl0

This comment has been minimized.

Copy link
Member

issyl0 commented Dec 2, 2019

When do people (not) want tinsel bunting to appear? Should it only be a Christmas Day/Boxing Day/New Year's Day thing?

@jennyd

This comment has been minimized.

Copy link
Contributor Author

jennyd commented Dec 2, 2019

When do people (not) want tinsel bunting to appear? Should it only be a Christmas Day/Boxing Day/New Year's Day thing?

Possibly debatable, but I'd say so - it seems odd to have tinsel for St Andrew's Day only in the years when there's a substitute bank holiday, and for it to have different behaviour from St Patrick's Day.

@@ -67,4 +67,12 @@ def content_id
def body
@data["body"]
end

def christmas?

This comment has been minimized.

Copy link
@bevanloon

bevanloon Dec 3, 2019

Contributor

I'm being very pedantic, but do we think this should be christmas_or_boxing_day?

This comment has been minimized.

Copy link
@issyl0

issyl0 Dec 3, 2019

Member

Boxing Day counts as Christmas for me.

But will this even work? Do the Christmas and Boxing Day bank holidays ever get substituted for not 25th/26th?

This comment has been minimized.

Copy link
@injms

injms Dec 3, 2019

If substituted, I think the most it could be different is two days out because of a weekend - so maybe [25, 26, 27, 28]?

This comment has been minimized.

Copy link
@jennyd

jennyd Dec 3, 2019

Author Contributor

Yes, all the bank holidays over Christmas/new year can be substitute days if the actual day falls on a weekend - so 25-28th Dec and 1st-4th Jan can be bank holidays that should have tinsel.

This comment has been minimized.

Copy link
@issyl0

issyl0 Dec 3, 2019

Member

Thanks! I pushed two extra commits to your branch that fixes the problem. Feel free to squash them. I didn't want to force-push your branch to do it myself.

This comment has been minimized.

Copy link
@jennyd

jennyd Dec 3, 2019

Author Contributor

Happy for you to squash and force-push - don't want to write your commit messages for you @issyl0 !

This comment has been minimized.

Copy link
@issyl0

issyl0 Dec 3, 2019

Member

Cool, I'm done now.

@bevanloon

This comment has been minimized.

Copy link
Contributor

bevanloon commented Dec 3, 2019

When do people (not) want tinsel bunting to appear? Should it only be a Christmas Day/Boxing Day/New Year's Day thing?

My vote is Christmas Day, Boxing Day and New Year's Day.

@issyl0 issyl0 force-pushed the jennyd:failing-test-tinsel-st-andrews-day branch 3 times, most recently from d3553d4 to 2fa2f6d Dec 3, 2019
- Previously, we showed tinsel on all bank holidays that fell in
  December and January, assuming that they were only Christmas Day,
  Boxing Day and New Year's Day.
- This year (and presumably other years in the past/future), St Andrew's
  Day in Scotland was a substitute day on 2nd December. We showed
  tinsel, when we should have just showed normal bunting.
- This also accounts for all possible substitute Christmas/New Year bank
  holidays.
@huwd
huwd approved these changes Dec 3, 2019
@issyl0 issyl0 merged commit 6410e90 into alphagov:master Dec 3, 2019
3 checks passed
3 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/publishing-e2e-tests Publishing end-to-end tests succeeded on Jenkins
Details
continuous-integration/jenkins/security No security issues found
Details
@issyl0

This comment has been minimized.

Copy link
Member

issyl0 commented Dec 3, 2019

Thank you for adding the test that made us realise this was a problem, @jennyd! 💪❤️

May there only be tinsel on legit Christmas and New Year bank holidays from now on! 🎄

@jennyd

This comment has been minimized.

Copy link
Contributor Author

jennyd commented Dec 3, 2019

And thank you for responding quickly and fixing it! 💥 🎉 :shipit: ❤️

(I still don't know why the test passes without the fix if you change it to use 2nd December 2019 - the behaviour we were seeing yesterday clearly shouldn't pass the test 🤔 )

@jennyd jennyd deleted the jennyd:failing-test-tinsel-st-andrews-day branch Dec 3, 2019
@rjw1

This comment has been minimized.

Copy link

rjw1 commented Dec 3, 2019

@jennyd

This comment has been minimized.

Copy link
Contributor Author

jennyd commented Dec 3, 2019

@jennyd the fixture for the tests being old maybe https://github.com/alphagov/calendars/blob/master/test/fixtures/data/bank-holidays.json

Ah good spot @rjw1, that'll be it - I'd assumed the tests were using the real data.

@brenetic

This comment has been minimized.

Copy link
Contributor

brenetic commented Dec 3, 2019

Timecop.travel(2017, 1, 1, 9, 0, 0)
group_hug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.