Add named params and yard docs to create_milestone method at GithubHelper
#426
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -217,31 +217,77 @@ def get_milestone(milestone_name:) | |
| allow(Octokit::Client).to receive(:new).and_return(client) | ||
| end | ||
|
|
||
| it 'computes the correct dates for standard period' do | ||
| due_date = '2022-10-20T08:00:00Z'.to_time.utc | ||
|
There was a problem hiding this comment. This example date feels dangerously close to the actual DST change date zone—e.g. in Europe, DST starts on the last Sunday of March and ends on the last Sunday of October, so it happened on Oct 30 this year. Which is still outside the Oct 20–Oct 27 date range that is used in this example, but still very close. |
||
| options = { | ||
| due_on: '2022-10-20T12:00:00Z', | ||
| description: "Code freeze: October 20, 2022\nApp Store submission: October 24, 2022\nRelease: October 27, 2022\n" | ||
| } | ||
|
|
||
| expect(client).to receive(:create_milestone).with(test_repo, test_milestone_number, options) | ||
| create_milestone(due_date: due_date, days_until_submission: 4, days_until_release: 7) | ||
| end | ||
|
|
||
| it 'computes the correct dates when the due date is on the verge of a DST day change' do | ||
| # The PST to PDT (DST) change is made on the second Sunday in March and finishes on the first Sunday in November | ||
| Time.zone = 'Pacific Time (US & Canada)' | ||
| due_date = '2022-06-18T23:00:00Z'.to_time | ||
|
There was a problem hiding this comment.
Then that means that the date you used ( There was a problem hiding this comment. I wanted to check the PST to PDT details and landed on Wikipedia. Here's the link for future reference, just in case. There was a problem hiding this comment. I had problems testing this properly, to be honest. It confuses me that the fact that PST/PDT changes are made at 2 am, Also for some reason, the date to not change when I use the 14 march (the actual DST change date) as my date, even forcing the timezone to be at PST. So I just used any date inside the DST changes, given the date that was put was on UTC. There was a problem hiding this comment. Due to the Ci failure, I think that this is not the proper way to enforce the use of a specific timezone (although it passes here on my local test run). I'll try to build it differently, I was researching other ways to do this and found this one, which is wrapping the test on the timezone change block: Time.use_zone('Europe/London') do
due_date = Time.zone.parse('2022-03-27 23:00:00')
# ...
endTBH, I'm not sure if this is the correct way to do it either, as there are a lot of different answers on how to do this (most of them on rails), on StackOverflow. If you folks have suggestions on how to deal with this, I would love to hear @AliSoftware @mokagio 😄 Note: I'll use the DST from Europe this time, that makes more sense in my head than the American ones. 😅 There was a problem hiding this comment. From the top of my head I don't remember what's the right way to do it, though |
||
| options = { | ||
| due_on: '2022-06-19T12:00:00Z', | ||
| description: "Code freeze: June 19, 2022\nApp Store submission: June 21, 2022\nRelease: June 22, 2022\n" | ||
| } | ||
|
|
||
| expect(client).to receive(:create_milestone).with(test_repo, test_milestone_number, options) | ||
| create_milestone(due_date: due_date, days_until_submission: 2, days_until_release: 3) | ||
| end | ||
|
|
||
| it 'computes the correct dates when the due date is on DST but has no day change' do | ||
| # The PST to PDT (DST) change is made on the second Sunday in March and finishes on the first Sunday in November | ||
| Time.zone = 'Pacific Time (US & Canada)' | ||
| due_date = '2022-06-18T22:00:00Z'.to_time | ||
| options = { | ||
| due_on: '2022-06-18T12:00:00Z', | ||
| description: "Code freeze: June 18, 2022\nApp Store submission: June 20, 2022\nRelease: June 21, 2022\n" | ||
| } | ||
|
|
||
| expect(client).to receive(:create_milestone).with(test_repo, test_milestone_number, options) | ||
| create_milestone(due_date: due_date, days_until_submission: 2, days_until_release: 3) | ||
| end | ||
|
|
||
| context 'with input validation' do | ||
|
There was a problem hiding this comment. Out curiosity, why did you choose to use a There was a problem hiding this comment. I thought that will be more organized and improve the readability to have the input validations under the context, instead of having them grouped under the main describe block. Although, now that you ask that question, it made me think if this is really useful at all 🤔 There was a problem hiding this comment. I think Gio's question was more about why using There was a problem hiding this comment. @raafaelima @AliSoftware, a bit of both 😄 Why grouping the input validation? (You answered that already) And why with It is my understanding that In particular, the example from the docs use RSpec.describe "something" do
context "when something is a certain way" do
it "does something" do
# example code goes here
end
end
endThat pattern matches my experience with and mental model of There was a problem hiding this comment. Some additional doc / reference about |
||
| it 'raises an error if days_until_submission is less than or equal zero' do | ||
| due_date = '2022-10-20T08:00:00Z'.to_time.utc | ||
| expect { create_milestone(due_date: due_date, days_until_submission: 0, days_until_release: 5) } | ||
| .to raise_error(FastlaneCore::Interface::FastlaneError, 'days_until_submission must be greater than zero.') | ||
| end | ||
|
|
||
| it 'raises an error if days_until_release is less than or equal zero' do | ||
| due_date = '2022-10-20T08:00:00Z'.to_time.utc | ||
| expect { create_milestone(due_date: due_date, days_until_submission: 12, days_until_release: -8) } | ||
| .to raise_error(FastlaneCore::Interface::FastlaneError, 'days_until_release must be greater than zero.') | ||
| end | ||
|
|
||
| it 'raises an error if submission date is after the release date' do | ||
| due_date = '2022-10-20T08:00:00Z'.to_time.utc | ||
| expect { create_milestone(due_date: due_date, days_until_submission: 14, days_until_release: 3) } | ||
| .to raise_error(FastlaneCore::Interface::FastlaneError, 'days_until_release must be greather than days_until_submission') | ||
| end | ||
|
|
||
| it 'raises an error if submission date is equal to release date' do | ||
| due_date = '2022-10-20T08:00:00Z'.to_time.utc | ||
| expect { create_milestone(due_date: due_date, days_until_submission: 1, days_until_release: 1) } | ||
| .to raise_error(FastlaneCore::Interface::FastlaneError, 'days_until_release must be greather than days_until_submission') | ||
| end | ||
|
There was a problem hiding this comment. This should be a valid case imho (see other comment) There was a problem hiding this comment. Yeah, I was a bit confused if this should be or not be a valid case, Initially, I think it was, but as you explained here about the process, it feels that the submissions always need to be before release, and not fall in the same day. I'll change that test to be a valid case, thanks for the input. 😄 |
||
| end | ||
|
|
||
| def create_milestone(due_date:, days_until_submission:, days_until_release:) | ||
| helper = described_class.new(github_token: 'Fake-GitHubToken-123') | ||
| helper.create_milestone( | ||
| repository: test_repo, | ||
| title: test_milestone_number, | ||
| due_date: due_date, | ||
| days_until_submission: days_until_submission, | ||
| days_until_release: days_until_release | ||
| ) | ||
| end | ||
| end | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels to me that this would be more readable if the conditions matched the messages described in the
user_error!, rather than being the negative of those ("must be greater than zero" --> use adays_until-release > 0condition), otherwise it needs your brain to think what the negative should be to ensure that theifis correct and matching the message.In fact, for the 3rd case I think the condition and messages are incorrect, because it could be valid to consider that the submission and the release could be on the same day (that's not a common/used setup for any of our apps currently, but might be one day, at least it shouldn't be invalid imho)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback, I was not aware of the
unlesskeyword, indeed is more legible that way. 😄