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.

Sign up for GitHub

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

Jump to bottom

Add Unit tests for milestone actions #425

Open
wants to merge 12 commits into
base: trunk
Choose a base branch
from
Open

Add Unit tests for milestone actions #425

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c37f29e
CloseMilestoneAction: Add UnitTests
raafaelima Nov 3, 2022
6dfd767
CreateNewMilestoneAction: Add UnitTests
raafaelima Nov 3, 2022
22b9d44
SetFrozenTagAction: Add UnitTests
raafaelima Nov 3, 2022
96acb93
SetFrozenTagSpec: Fix Failing Tests due the introduce the Hash-splat …
raafaelima Nov 3, 2022
f695b04
Merge branch 'wordpress-mobile:trunk' into add/unit-tests-for-milesto…
raafaelima Nov 3, 2022
3472f9e
CloseMilestoneAction: Fix Issues Pointed on PR
raafaelima Nov 4, 2022
cdd96c2
SetFrozenTagAction: Fix Issues Pointed on PR
raafaelima Nov 4, 2022
3a5a240
CreateNewMilestoneAction: Fix Issues Pointed on PR
raafaelima Nov 4, 2022
5e656dd
Specs: Add Shared Examples for Actions that require Github_Token
raafaelima Nov 6, 2022
b6de4cd
CloseMilestoneAction: Add Shared Examples of GithubToken
raafaelima Nov 6, 2022
9eb95c2
CreateMilestoneAction: Add Shared Examples of GithubToken
raafaelima Nov 6, 2022
87ae1d4
SetFrozenTagAction: Add Shared Examples of GithubToken
raafaelima Nov 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 105 additions & 0 deletions spec/close_milestone_action_spec.rb
View file
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that let declarations are re-evaluated for each "example" (aka it test case), so it shouldn't be necessary to reset mock_params[:github_token] to nil before 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 those mock_params with any additional values when you need those in your test cases below.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, as per my suggestion above, what I'd do instead here would be something like:

Suggested change
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:

Suggested change
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:

Suggested change
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"?


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

describe 'get_milestone' do
Copy link
Contributor

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 🤔

let(:test_repository) { 'test-repository' }
let(:test_milestone) { '10' }
let(:mock_params) do
{
repository: test_repository,
milestone: test_milestone,
github_token: 'Test-GithubToken-1234'
}
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

it 'properly passes the repository all the way down to the Octokit::Client to list the milestones' do
allow(Octokit::Client).to receive(:new).and_return(client)
expect(client).to receive(:list_milestones).with(test_repository)
run_described_fastlane_action(mock_params)
end
Copy link
Contributor

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).

end

describe 'update_milestone' do
let(:test_repository) { 'test-repository' }
let(:test_milestone_number) { '1234' }
let(:mock_params) do
{
repository: test_repository,
milestone: '10',
github_token: 'Test-GithubToken-1234'
}
end
let(:client) do
instance_double(
Octokit::Client,
list_milestones: [{ title: '10.1', number: test_milestone_number }],
update_milestone: nil,
user: instance_double('User', name: 'test'),
'auto_paginate=': nil
)
end

it 'properly passes the parameters all the way down to Octokit::Client' do
allow(Octokit::Client).to receive(:new).and_return(client)
expect(client).to receive(:update_milestone).with(test_repository, test_milestone_number, { state: 'closed' })
run_described_fastlane_action(mock_params)
end
end
end
156 changes: 156 additions & 0 deletions spec/create_new_milestone_action_spec.rb
View file
Open in desktop
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.
Show resolved Hide resolved
let(:glithub_helper) do
raafaelima marked this conversation as resolved.
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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 ConfigItem parameter names for that one… those are quite a mouthful currently 😅

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.
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again given this is the spec for create_new_milestone_action_spec, we should focus this test on testing the end result of the action, not the intermediate steps from internal details.

I'm not even sure when reading that test that I'd understand why we test get_last_milestone (isn't that the name of a separate and completely different action?) here.

If anything, what we want to test for the CreateNewMilestoneAction around list_milestones might be that the new milestone that it is about to create is based on the correct last milestone + the correct date computation from the due date of the last milestone and the day offsets. This is Unit Tests, so imho they should test the grand scheme of things from a public API point of view, not unit-test the details of the internal calls made in the implementation. So we should have a test like it 'computes the date of the new milestone based on the last milestone and correct date offsets' and that test should allow(client).to receive(:list_milestones).and_return(mock_milestones_list), then expect(client).to receive(:create_milestone).with(…the right params and especially the right due_on and description…), so that we unit-test that the due_on and other dates are correctly computed from the expected latest milestone found in mock_milestones_list + the correct day offsets.
That's what we really want to test when unit-testing the public API and the general behavior of the action from a black-box perspective.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the really useful unit test we need when testing the CreateNewMilestoneAction as a black box: pass some input parameters, and test that the create_milestone method of the Octokit::Client is called with the correct and expected parameters and final dates.

Except that there are so many more possible inputs to cover using that test case, especially:

  • Test that the computed dates (in due_on and description) are correct and the computation of the new dates were based on the right milestone (i.e. when there are multiple milestones returned by list_milestones, with different due_on dates, ensure the final dates for the new milestone is still correct)
  • Test the case where no milestones were returned
  • Test the case when we pass different offsets
  • Test error cases when we pass inconsistent or invalid offsets, if relevant
  • Test the case when the last milestone is just before a DST change, to ensure our date math takes that into account
  • …

Those are the use cases we need to cover, and they should basically all have the same structure as this it unit test above (i.e. their goal is always to expect(client).to receive(:create_milestone).with(the-correct-params) given the params passed to run_described_fastlane_action), but just with different inputs (different list of returned milestones, existing milestines with different due dates or with missing due dates or whatnot, case of need_appstore_submission true vs false, case of different day offsets, …) and thus different corresponding expected parameters passed to the client.

Copy link
Contributor Author

@raafaelima raafaelima Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I agree that those tests should be made as they are indeed valuable. Shouldn't all this validation about the date offsets, DST, and general due date/description cases that you mention be handled by the Unit tests from GithubHelper instead of the ones from the Action itself? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't all this validation about the date offsets, DST, and general due date/description cases that you mention be handled by the Unit tests from GithubHelper instead of the ones from the Action itself? 🤔

That's a good point, indeed.

Thought the implementation of this action (and its split of the code between the action itself and the helper) is apparently currently at an odd place, because some of the date math is done in the action (namely newmilestone_duedate = (milestone_duedate.to_datetime.next_day(milestone_duration).to_time).utc), while other parts of the date math is done in GithubHelper#create_milestone itself (namely the computation of submission_date and release_date based on days_until_submission and number_of_days_from_code_freeze_to_release)…

We should clean that up at some point (the API of that CreateNewMilestoneAction is a bit odd anyway and we hope to modernize it at one point to make the ConfigItem it declares have better names and API… but that's another story for another time) and once we do, we'd probably have all the date math be done in the action and not in the helper (which would just receive the already-computed dates as input then), which would then make testing the date math in the create_new_milestone_action_spec the next sensible thing.

That being said, I see you added some tests about this in lines 30–57 since 👍

end
end
136 changes: 136 additions & 0 deletions spec/setfrozentag_action_spec.rb
View file
Open in desktop
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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-Review: This is also a point where I have doubts about the pattern used across the app, I leave here the name of the method that Oktokit should call to pass the correct data. I was wondering at this moment if I should instead have done as:

describe 'run' do #or executeAction (naming is hard here)
   it 'update_milestone properly passes parameters all the way down to the Octokit::Client'
     ...
   end
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah as commented above, I'm having a hard time following the logic of how you split and organized the tests tbh.

Here since the goal is to unit test the SetfrozentagAction, why do we even have a describe 'update_milestone' (which relates to an API method here, while describe is usually more to group together multiple unit tests that test one common thing but stressing it in multiple ways)?

What we want to test in setfrozentag_action_spec is that, given various different inputs passed to the action (different parameters passed to run_described_fastlane_action(…)), the final outcome is what's expected — i.e. the final method call on the Octokit::Client that updates the milestone with the frozen emoji updates the right milestone, and with the right expected title. And, that if we pass invalid parameters or encounter an error or corner case (e.g. we're asking the action to setfrozentag(milestone: '10.1', …) but no milestone with title 10.1 exists), it handles it properly/nicely instead of doing something unexpected (like modifying another milestone or crashing badly).

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