-
Notifications
You must be signed in to change notification settings - Fork 8
Add Unit tests for milestone actions #425
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
base: trunk
Are you sure you want to change the base?
Changes from 3 commits
c37f29e
6dfd767
22b9d44
96acb93
f695b04
3472f9e
cdd96c2
3a5a240
5e656dd
b6de4cd
9eb95c2
87ae1d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,105 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| require 'spec_helper' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| describe Fastlane::Actions::CloseMilestoneAction do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| describe 'initialize' do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let(:test_token) { 'Test-GithubToken-1234' } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let(:mock_params) do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| repository: 'test-repository', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| milestone: '10' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let(:client) do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| instance_double( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Octokit::Client, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| list_milestones: [{ title: '10.1' }], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| update_milestone: nil, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| user: instance_double('User', name: 'test'), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'auto_paginate=': nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| before do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ENV['GITHUB_TOKEN'] = nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mock_params[:github_token] = nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| allow(Octokit::Client).to receive(:new).and_return(client) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it 'properly passes the environment variable `GITHUB_TOKEN` all the way to Octokit::Client' do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ENV['GITHUB_TOKEN'] = test_token | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(Octokit::Client).to receive(:new).with(access_token: test_token) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| run_described_fastlane_action(mock_params) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it 'properly passes the parameter `:github_token` all the way to Octokit::Client' do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mock_params[:github_token] = test_token | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(Octokit::Client).to receive(:new).with(access_token: test_token) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| run_described_fastlane_action(mock_params) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it 'properly passes the parameter `:github_token` all the way to Octokit::Client' do | |
| mock_params[:github_token] = test_token | |
| expect(Octokit::Client).to receive(:new).with(access_token: test_token) | |
| run_described_fastlane_action(mock_params) | |
| end | |
| it 'properly passes the parameter `:github_token` all the way to Octokit::Client' do | |
| expect(Octokit::Client).to receive(:new).with(access_token: test_token) | |
| run_described_fastlane_action(mock_params.merge({github_token: test_token})) | |
| end |
Or, because Ruby's Hash#merge method uses a similar trick to what you used in #424 to allow last-parameter options to be provided by a list of additional keyword parameters instead of a formal Hash, you can also omit the {} in merge and write it like this:
| it 'properly passes the parameter `:github_token` all the way to Octokit::Client' do | |
| mock_params[:github_token] = test_token | |
| expect(Octokit::Client).to receive(:new).with(access_token: test_token) | |
| run_described_fastlane_action(mock_params) | |
| end | |
| it 'properly passes the parameter `:github_token` all the way to Octokit::Client' do | |
| expect(Octokit::Client).to receive(:new).with(access_token: test_token) | |
| run_described_fastlane_action(mock_params.merge(github_token: test_token)) | |
| end |
Or alternatively, use the "Hash-splat" operator in reverse here, making it expand the mock_params Hash as if its keys and values were provided as a list of named parameters directly:
| it 'properly passes the parameter `:github_token` all the way to Octokit::Client' do | |
| mock_params[:github_token] = test_token | |
| expect(Octokit::Client).to receive(:new).with(access_token: test_token) | |
| run_described_fastlane_action(mock_params) | |
| end | |
| it 'properly passes the parameter `:github_token` all the way to Octokit::Client' do | |
| expect(Octokit::Client).to receive(:new).with(access_token: test_token) | |
| run_described_fastlane_action(**mock_params, github_token: test_token) | |
| end |
Whatever option you use, the point is to avoid modifying mock_params because even if Ruby technically allows you to do it, it should be considered as a constant (hence the let keyword convention) in principle (and modifying it in-place makes it harder to reason about imho).
PS: If you adopt this approach everywhere you need to adjust mock_params, you might consider renaming that constant to let(:default_params) (or let(:common_params)?) instead of let(:mock_params), to make it clear that this is the "baseline of the params commonly used in all the test cases"?
Outdated
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.
I'm a bit confused here because this announces that the test cases in that group will test get_milestone, but I don't see anything related to get_milestone in the it test cases below 🤔
Outdated
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.
If this is really supposed to be testing the actual CloseMilestoneAction described action (since it calls run_described_fastlane_action), and is supposed to be about get_milestone (since that's what the describe group is announcing on line 52 above), then why is the expectation on list_milestones?
Tbh I feel like the organization of those tests (i.e. the split you used for the describe groups) is a bit confusing. I'm not sure it makes sense to have a whole describe 'get_milestone' and describe 'update_milestone' etc, and each with only one it test case; besides the describe and the test they contain don't always seem to be consistent or to make the best of sense to me?
Since the goal here is to test that the action has the expected end result, regardless of the intermediate steps it needs to achieve this internally, I'd instead suggest to focus the unit test on exactly that, i.e. call run_described_fastlane_action with a given repo and milestone, and test that the right method of Octokit::Client gets called, with the correct parameters. Or that the action errors if we pass it incorrect input parameters.
So basically, I think the only useful test we really need for the happy path is the one on line 99 (which probably then doesn't really need its own describe just for a single it).
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,156 @@ | ||
| require 'spec_helper' | ||
|
|
||
| describe Fastlane::Actions::CreateNewMilestoneAction do | ||
| describe 'initialize' do | ||
| let(:test_token) { 'Test-GithubToken-1234' } | ||
| let(:test_milestone) { { title: '10.1', due_on: '2022-10-31T07:00:00Z' } } | ||
| let(:mock_params) do | ||
| { | ||
| repository: 'test-repository', | ||
| need_appstore_submission: false | ||
| } | ||
| end | ||
| let(:client) do | ||
| instance_double( | ||
| Octokit::Client, | ||
| list_milestones: [test_milestone], | ||
| create_milestone: nil, | ||
| user: instance_double('User', name: 'test'), | ||
| 'auto_paginate=': nil | ||
| ) | ||
| end | ||
|
|
||
| before do | ||
| ENV['GITHUB_TOKEN'] = nil | ||
| mock_params[:github_token] = nil | ||
| allow(Octokit::Client).to receive(:new).and_return(client) | ||
| end | ||
|
|
||
| context 'with github_token' do | ||
| it 'properly passes the environment variable `GITHUB_TOKEN` all the way to Octokit::Client' do | ||
| ENV['GITHUB_TOKEN'] = test_token | ||
| expect(Octokit::Client).to receive(:new).with(access_token: test_token) | ||
| run_described_fastlane_action(mock_params) | ||
| end | ||
|
|
||
| it 'properly passes the parameter `:github_token` all the way to Octokit::Client' do | ||
| mock_params[:github_token] = test_token | ||
| expect(Octokit::Client).to receive(:new).with(access_token: test_token) | ||
| run_described_fastlane_action(mock_params) | ||
| end | ||
|
|
||
| it 'prioritizes `:github_token` parameter over `GITHUB_TOKEN` enviroment variable if both are present' do | ||
| ENV['GITHUB_TOKEN'] = 'Test-EnvGithubToken-1234' | ||
| mock_params[:github_token] = test_token | ||
| expect(Octokit::Client).to receive(:new).with(access_token: test_token) | ||
| run_described_fastlane_action(mock_params) | ||
| end | ||
|
|
||
| it 'prints an error if no `GITHUB_TOKEN` environment variable nor parameter `:github_token` is present' do | ||
| expect { run_described_fastlane_action(mock_params) }.to raise_error(FastlaneCore::Interface::FastlaneError) | ||
| end | ||
| end | ||
|
|
||
| context 'with default parameters' do | ||
raafaelima marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| let(:glithub_helper) do | ||
raafaelima marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| instance_double( | ||
| Fastlane::Helper::GithubHelper, | ||
| get_last_milestone: test_milestone, | ||
| create_milestone: nil | ||
| ) | ||
| end | ||
|
|
||
| before do | ||
| mock_params[:github_token] = test_token | ||
| allow(Fastlane::Helper::GithubHelper).to receive(:new).and_return(glithub_helper) | ||
| end | ||
|
|
||
| it 'uses default value when neither `GHHELPER_NUMBER_OF_DAYS_FROM_CODE_FREEZE_TO_RELEASE` environment variable nor parameter `:number_of_days_from_code_freeze_to_release` is present' do | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gosh we really need to refactor the API of this action to use nicer and more comprehensible / consistent One day… in another PR… in the far future… maybe… |
||
| default_code_freeze_days = 14 | ||
| mock_params[:number_of_days_from_code_freeze_to_release] = nil | ||
| ENV['GHHELPER_NUMBER_OF_DAYS_FROM_CODE_FREEZE_TO_RELEASE'] = nil | ||
| expect(glithub_helper).to receive(:create_milestone).with( | ||
| anything, | ||
| anything, | ||
| anything, | ||
| anything, | ||
| default_code_freeze_days, | ||
| anything | ||
| ) | ||
raafaelima marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| run_described_fastlane_action(mock_params) | ||
| end | ||
|
|
||
| it 'uses default value when neither `GHHELPER_MILESTONE_DURATION` environment variable nor parameter `:milestone_duration` is present' do | ||
| default_milestone_duration = 14 | ||
| mock_params[:milestone_duration] = nil | ||
| ENV['GHHELPER_MILESTONE_DURATION'] = nil | ||
| expect(glithub_helper).to receive(:create_milestone).with( | ||
| anything, | ||
| anything, | ||
| anything, | ||
| default_milestone_duration, | ||
| anything, | ||
| anything | ||
| ) | ||
| run_described_fastlane_action(mock_params) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe 'get_last_milestone' do | ||
|
||
| let(:test_repository) { 'test-repository' } | ||
| let(:test_milestone) { { title: '10.1', due_on: '2022-10-31T07:00:00Z' } } | ||
| let(:mock_params) do | ||
| { | ||
| repository: test_repository, | ||
| need_appstore_submission: false, | ||
| github_token: 'Test-GithubToken-1234' | ||
| } | ||
| end | ||
| let(:client) do | ||
| instance_double( | ||
| Octokit::Client, | ||
| list_milestones: [test_milestone], | ||
| create_milestone: nil, | ||
| user: instance_double('User', name: 'test'), | ||
| 'auto_paginate=': nil | ||
| ) | ||
| end | ||
|
|
||
| it 'properly passes the repository all the way down to the Octokit::Client to list the existing milestones' do | ||
| allow(Octokit::Client).to receive(:new).and_return(client) | ||
| expect(client).to receive(:list_milestones).with(test_repository, { state: 'open' }) | ||
| run_described_fastlane_action(mock_params) | ||
| end | ||
| end | ||
|
|
||
| describe 'create_milestone' do | ||
| let(:test_repository) { 'test-repository' } | ||
| let(:test_milestone_number) { '10.2' } | ||
| let(:test_milestone) { { title: '10.1', due_on: '2022-10-31T07:00:00Z' } } | ||
| let(:mock_params) do | ||
| { | ||
| repository: test_repository, | ||
| need_appstore_submission: false, | ||
| github_token: 'Test-GithubToken-1234' | ||
| } | ||
| end | ||
| let(:client) do | ||
| instance_double( | ||
| Octokit::Client, | ||
| list_milestones: [test_milestone], | ||
| create_milestone: nil, | ||
| user: instance_double('User', name: 'test'), | ||
| 'auto_paginate=': nil | ||
| ) | ||
| end | ||
|
|
||
| it 'properly passes the parameters all the way down to the Octokit::Client' do | ||
| comment = "Code freeze: November 14, 2022\nApp Store submission: November 28, 2022\nRelease: November 28, 2022\n" | ||
| options = { due_on: '2022-11-14T12:00:00Z', description: comment } | ||
| allow(Octokit::Client).to receive(:new).and_return(client) | ||
| expect(client).to receive(:create_milestone).with(test_repository, test_milestone_number, options) | ||
| run_described_fastlane_action(mock_params) | ||
| end | ||
|
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| require 'spec_helper' | ||
|
|
||
| describe Fastlane::Actions::SetfrozentagAction do | ||
| describe 'initialize' do | ||
| let(:test_token) { 'Test-GithubToken-1234' } | ||
| let(:test_milestone) { { title: '10.1', number: 1234 } } | ||
| let(:mock_params) do | ||
| { | ||
| repository: 'test-repository', | ||
| milestone: test_milestone[:title] | ||
| } | ||
| end | ||
| let(:client) do | ||
| instance_double( | ||
| Octokit::Client, | ||
| list_milestones: [test_milestone], | ||
| update_milestone: nil, | ||
| user: instance_double('User', name: 'test'), | ||
| 'auto_paginate=': nil | ||
| ) | ||
| end | ||
|
|
||
| before do | ||
| ENV['GITHUB_TOKEN'] = nil | ||
| mock_params[:github_token] = nil | ||
| allow(Octokit::Client).to receive(:new).and_return(client) | ||
| end | ||
|
|
||
| context 'with github_token' do | ||
| it 'properly passes the environment variable `GITHUB_TOKEN` all the way to Octokit::Client' do | ||
| ENV['GITHUB_TOKEN'] = test_token | ||
| expect(Octokit::Client).to receive(:new).with(access_token: test_token) | ||
| run_described_fastlane_action(mock_params) | ||
| end | ||
|
|
||
| it 'properly passes the parameter `:github_token` all the way to Octokit::Client' do | ||
| mock_params[:github_token] = test_token | ||
| expect(Octokit::Client).to receive(:new).with(access_token: test_token) | ||
| run_described_fastlane_action(mock_params) | ||
| end | ||
|
|
||
| it 'prioritizes `:github_token` parameter over `GITHUB_TOKEN` enviroment variable if both are present' do | ||
| ENV['GITHUB_TOKEN'] = 'Test-EnvGithubToken-1234' | ||
| mock_params[:github_token] = test_token | ||
| expect(Octokit::Client).to receive(:new).with(access_token: test_token) | ||
| run_described_fastlane_action(mock_params) | ||
| end | ||
|
|
||
| it 'prints an error if no `GITHUB_TOKEN` environment variable nor parameter `:github_token` is present' do | ||
| expect { run_described_fastlane_action(mock_params) }.to raise_error(FastlaneCore::Interface::FastlaneError) | ||
| end | ||
| end | ||
|
|
||
| context 'with default parameters' do | ||
| let(:glithub_helper) do | ||
| instance_double( | ||
| Fastlane::Helper::GithubHelper, | ||
| get_milestone: test_milestone, | ||
| create_milestone: nil | ||
| ) | ||
| end | ||
|
|
||
| before do | ||
| mock_params[:github_token] = test_token | ||
| allow(Fastlane::Helper::GithubHelper).to receive(:new).and_return(glithub_helper) | ||
| end | ||
|
|
||
| it 'froze the milestone when the parameter `:freeze` is not present' do | ||
| default_freeze_milestone = { title: '10.1 ❄️' } | ||
| expect(glithub_helper).to receive(:update_milestone).with( | ||
| repository: anything, | ||
| number: anything, | ||
| options: default_freeze_milestone | ||
| ) | ||
| run_described_fastlane_action(mock_params) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe 'get_milestone' do | ||
| let(:test_repository) { 'test-repository' } | ||
| let(:test_milestone) { { title: '10.1', number: 1234 } } | ||
| let(:mock_params) do | ||
| { | ||
| repository: test_repository, | ||
| milestone: test_milestone[:title], | ||
| github_token: 'Test-GithubToken-1234' | ||
| } | ||
| end | ||
| let(:client) do | ||
| instance_double( | ||
| Octokit::Client, | ||
| list_milestones: [test_milestone], | ||
| update_milestone: nil, | ||
| user: instance_double('User', name: 'test'), | ||
| 'auto_paginate=': nil | ||
| ) | ||
| end | ||
|
|
||
| before do | ||
| allow(Octokit::Client).to receive(:new).and_return(client) | ||
| end | ||
|
|
||
| it 'properly passes parameters all the way down to the Octokit::Client' do | ||
| expect(client).to receive(:list_milestones).with(test_repository) | ||
| run_described_fastlane_action(mock_params) | ||
| end | ||
| end | ||
|
|
||
| describe 'update_milestone' do | ||
|
||
| let(:test_repository) { 'test-repository' } | ||
| let(:test_milestone) { { title: '10.1', number: 1234 } } | ||
| let(:mock_params) do | ||
| { | ||
| repository: test_repository, | ||
| milestone: test_milestone[:title], | ||
| github_token: 'Test-GithubToken-1234' | ||
| } | ||
| end | ||
| let(:client) do | ||
| instance_double( | ||
| Octokit::Client, | ||
| list_milestones: [test_milestone], | ||
| update_milestone: nil, | ||
| user: instance_double('User', name: 'test'), | ||
| 'auto_paginate=': nil | ||
| ) | ||
| end | ||
|
|
||
| it 'properly passes parameters all the way down to the Octokit::Client' do | ||
| allow(Octokit::Client).to receive(:new).and_return(client) | ||
| expect(client).to receive(:update_milestone).with(test_repository, test_milestone[:number], { title: '10.1 ❄️' }) | ||
| run_described_fastlane_action(mock_params) | ||
| end | ||
| 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.
I think that
letdeclarations are re-evaluated for each "example" (akaittest case), so it shouldn't be necessary to resetmock_params[:github_token]tonilbefore each there.Besides, I'm not a fan of modifying what is conceptually a constant here. Instead of modifying
mock_params, I'd suggest you to build a new Hash merging thosemock_paramswith any additional values when you need those in your test cases below.