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 Unit tests for milestone actions #425

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

Conversation

raafaelima
Copy link
Contributor

@raafaelima raafaelima commented Nov 3, 2022

What does this solve?

With the changes/improvements made on PR #420, we discover that some actions were not properly covered by unit tests. On this PR we start adding them, starting with the milestone actions.

What does PR #420 was about?

As stated in issue #416, The CI was failing at the moment of doing a GitHub release, The reason for the failure was a 401 from the GitHub API. This happened because neither GHHELPER_ACCESS (that is not valid anymore) nor GITHUB_TOKEN was available in the environment at runtime.

We solved this by turning the GITHUB_TOKEN into a mandatory env variable/parameter to run those actions and changing the GithubHelper class to receive this token at their initialization, so we have a centralized entry-point at the GithubAPI.

Any Dependencies?

As this work is impacted by #426, Please, DO NOT merge this before it. Thanks! 😄

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

…operator

After introducing the Hash-splat operator some tests were failing because they are expecting the value of the options as an hash using the cruly braces.
Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

I have to admit that I have a hard time understanding the logic behind how you organized your tests (the way you decided to split your describe and it, why you only have one it unit test per describe while describe are usually to group multiple related tests together, …).

Besides, I don't think those tests are really testing what really matters, and they don't seem to cover the different cases (of validating the behavior of the actions under different values of input parameters and contexts) and edge cases (missing milestones, more than one milestone, tricky dates, …) that those tests would be useful to protect us against.


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.

Comment on lines 34 to 38
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"?

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 🤔

Comment on lines 72 to 76
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).

spec/create_new_milestone_action_spec.rb Outdated Show resolved Hide resolved
spec/create_new_milestone_action_spec.rb Outdated Show resolved Hide resolved
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.

Comment on lines 148 to 154
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

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

describe 'update_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.

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

@@ -49,19 +53,19 @@ def self.available_options
env_name: 'GHHELPER_NEED_APPSTORE_SUBMISSION',
description: 'True if the app needs to be submitted',
optional: true,
is_string: false,
type: Boolean,
Copy link
Contributor Author

@raafaelima raafaelima Nov 4, 2022

Choose a reason for hiding this comment

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

Self-Review: As the is_string is deprecated on the ConfigItems, I replace them with the type of variable that we want to receive. This also applies to the other places where I did that change.

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 great! It's even a small step towards addressing #278 that we opened a while ago about this 🙂

@raafaelima
Copy link
Contributor Author

Indeed I was too focused on the tests of GITHUB_TOKEN and too biased about using the existent tests of GithubHelper to guide me, that I end up using the same patterns (like the describe with method names). I made some changes and improve the legibility and organization of those tests, I hope now I have reached the right point.

Comment on lines 56 to 59
it 'does not freeze the milestone if :freeze parameter is false' do
expect(client).to receive(:update_milestone).with(test_repository, test_milestone[:number], title: '10.1')
run_action_with(:freeze, false)
end
Copy link
Contributor Author

@raafaelima raafaelima Nov 4, 2022

Choose a reason for hiding this comment

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

Self-Review: This for me is an odd behavior of this action If the :freeze parameter is false, it does not add the ❄️ symbol to the milestone title, as expected. However, it keeps calling the update_milestone to do an "unnecessary" update at that milestone.

Shouldn't in this case, the update_milestone not be called, following the same logic in place when the milestone is already frozen?

Copy link
Contributor

@AliSoftware AliSoftware Nov 4, 2022

Choose a reason for hiding this comment

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

The call is NOT unnecessary, as it will remove any existing ❄️ emoji from a frozen milestone title if it contains one — i.e. update milestone 20.9 ❄️ to 20.9. This is used when we finalize a release at the end of a sprint, just before closing the milestone (and freezing the next one on the next code freeze).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I didn't notice that behavior. 🤔
I add a test on it in any case, about removing any existing ❄️ emoji from a frozen milestone. 😄

@raafaelima raafaelima requested review from AliSoftware and removed request for mokagio November 4, 2022 01:58
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
it 'properly passes the repository and milestone to Octokit::Client to update the milestone as closed' do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it 'properly passes the repository and milestone to Octokit::Client to update the milestone as closed' do
it 'closes the expected milestone on the expected repository' do

end
it 'raises an error when the milestone is not found or does not exist' do
allow(client).to receive(:list_milestones).and_return([])
expect { run_described_fastlane_action(default_params) }.to raise_error(FastlaneCore::Interface::FastlaneError, 'Milestone 10.1 not found.')
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

end
end

def run_action_without_key(key)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
def run_action_without_key(key)
def run_action_without(key:)

Comment on lines 73 to 77
def run_action_with(key, value)
values = default_params
values[key] = value
run_described_fastlane_action(values)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: why not make this more flexible by allowing it to take a **options-style Hash as parameter so you can not only have a nicer call site, but also one that looks like it uses named parameters?

Suggested change
def run_action_with(key, value)
values = default_params
values[key] = value
run_described_fastlane_action(values)
end
def run_action_with(**keys_and_values)
params = default_params.merge(keys_and_values)
run_described_fastlane_action(params)
end

Should allow the call site to look like:

run_action_with(milestone: '10')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I see that you suggested that above, but forget to change it on the newest commit, sorry for that, Already change it 👍

Comment on lines 30 to 57
it 'computes the correct due date and milestone description' do
comment = "Code freeze: November 14, 2022\nApp Store submission: November 28, 2022\nRelease: November 28, 2022\n"
expect(client).to receive(:create_milestone).with(test_repository, '10.2', due_on: '2022-11-14T12:00:00Z', description: comment)
run_described_fastlane_action(default_params)
end

it 'removes 3 days from the AppStore submission date when `:need_appstore_submission` is true' do
comment = "Code freeze: November 14, 2022\nApp Store submission: November 25, 2022\nRelease: November 28, 2022\n"
expect(client).to receive(:create_milestone).with(test_repository, '10.2', due_on: '2022-11-14T12:00:00Z', description: comment)
run_action_with(:need_appstore_submission, true)
end

it 'uses the most recent milestone date to calculate the due date and version of new milestone' do
comment = "Code freeze: November 18, 2022\nApp Store submission: December 02, 2022\nRelease: December 02, 2022\n"
allow(client).to receive(:list_milestones).and_return(milestone_list)
expect(client).to receive(:create_milestone).with(test_repository, '10.5', due_on: '2022-11-18T12:00:00Z', description: comment)
run_described_fastlane_action(default_params)
end

it 'raises an error when the due date of milestone does not exists' do
allow(client).to receive(:list_milestones).and_return([{ title: '10.1', number: '1234' }])
expect { run_described_fastlane_action(default_params) }.to raise_error(FastlaneCore::Interface::FastlaneError, 'Milestone 10.1 has no due date.')
end

it 'raises an error when the milestone is not found or does not exist on the repository' do
allow(client).to receive(:list_milestones).and_return([])
expect { run_described_fastlane_action(default_params) }.to raise_error(FastlaneCore::Interface::FastlaneError, 'No milestone found on the repository.')
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

nit: how about grouping those in describe 'date computation is correct' (for the first 3) and describe 'raises error when it can't find last milestone' (or maybe context 'when last milestone cannot be used'?) (for the last 2) for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah! It will be more organized 😄

allow(Fastlane::Helper::GithubHelper).to receive(:new).and_return(github_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…

Comment on lines +94 to +101
expect(github_helper).to receive(:create_milestone).with(
anything,
anything,
anything,
anything,
default_code_freeze_days,
anything
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it wouldn't be the occasion to make this GithubHelper#create_milestone method start using named parameters instead of positional ones — that are even harder to identify in test cases like this amongst the other anything placeholders. That should then allow us to use .with(hash_including(code_freeze_days: 14)) instead of having to use all those anything placeholders there.

But that means a small refactoring of all the call sites though (which might be outside of this PR's scope… but depending on how big a change it would be—not sure there are that many call sites, the create_new_milestone action might even be the only one?—, that could still be worth it so we can then have both the call sites and those unit tests be way nicer…
I'll let you evaluate how much changes it would warrant to do this, and decide if it's worth it or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I see, the only places that will be affected are the GithubSpec and the create_milestone_action/Spec, I agree that this method should have the named params to "force" it to be better legible and cleaner.

However, TBH I do not feel that I should do it on this PR, as it is a bit out of scope (although is a small change). So, I'll open another PR to deal with this, and update these tests here once this new PR is merged 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: This is addressed on PR #426 😄

Comment on lines 143 to 159
def run_action_without_key(key)
run_described_fastlane_action(default_params.except(key))
end

def run_action_with(key, value)
values = default_params
values[key] = value
run_described_fastlane_action(values)
end

def default_params
{
repository: test_repository,
need_appstore_submission: false,
github_token: test_token
}
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Same suggestions as for the other spec — run_action_without(key:), run_action_with(**additional_keys_and_values), and let(:default_params)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if we can DRY those too using the shared_context, or if this will be too overkill, as we also need to have the default_params defined on another place, to share the operations with those two methods 🤔

Comment on lines 23 to 38
it 'properly passes the environment variable `GITHUB_TOKEN` to Octokit::Client' do
ENV['GITHUB_TOKEN'] = test_token
expect(Octokit::Client).to receive(:new).with(access_token: test_token)
run_action_without_key(:github_token)
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(default_params)
end

it 'prioritizes `:github_token` parameter over `GITHUB_TOKEN` environment variable if both are present' do
ENV['GITHUB_TOKEN'] = 'Test-EnvGithubToken-1234'
expect(Octokit::Client).to receive(:new).with(access_token: test_token)
run_described_fastlane_action(default_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.

Should be able to DRY those, or at least group them in a describe given how all 3 are related

AliSoftware
AliSoftware previously approved these changes Nov 7, 2022
Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback!

This is looking good so far 👍
I'd love to get @mokagio 's thoughts on it before merging if they have time.

Comment on lines +37 to +57
it 'freezes the milestone adding ❄️ to the title' do
expect(client).to receive(:update_milestone).with(test_repository, test_milestone[:number], title: '10.1 ❄️')
run_action_with(freeze: true)
end

it 'remove any existing ❄️ emoji from a frozen milestone' do
allow(client).to receive(:list_milestones).and_return([{ title: '10.2 ❄️', number: '1234' }])
expect(client).to receive(:update_milestone).with(test_repository, test_milestone[:number], title: '10.2')
run_action_with(freeze: false, milestone: '10.2')
end

it 'does not change a milestone that is already frozen' do
allow(client).to receive(:list_milestones).and_return([{ title: '10.2 ❄️', number: '1234' }])
expect(client).not_to receive(:update_milestone)
run_action_with(milestone: '10.2 ❄️')
end

it 'does not change an unfrozen milestone if :freeze parameter is false' do
expect(client).to receive(:update_milestone).with(test_repository, test_milestone[:number], title: '10.1')
run_action_with(freeze: false)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@AliSoftware AliSoftware self-assigned this Nov 7, 2022
@AliSoftware AliSoftware added the Do Not Merge e.g. if it depends on another PR to be merged first, or if that PR is only to trigger CI on forks, … label Nov 7, 2022
@AliSoftware AliSoftware dismissed their stale review November 7, 2022 16:12

The PR looks good right now, but I missed the fact that we were waiting for #426 to land first, to be able to rebase this #425 on top afterwards (since the PRs couldn't be stacked in the first place due to using a fork). So dismissing my approval—and adding "Do Not Merge" label—to ensure we don't merge the PR prematurely. Will re-review once the PR is rebased and updated post-#426 landing.

it 'properly passes the environment variable `GITHUB_TOKEN` to Octokit::Client' do
ENV['GITHUB_TOKEN'] = test_token
expect(Octokit::Client).to receive(:new).with(access_token: test_token)
run_action_without(:github_token)
Copy link
Contributor

Choose a reason for hiding this comment

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

This shared example relies on the spec including it (include_examples 'github_token_initialization') defining a run_action_without method and a default_params. The only way a consumer would find out about this would be by running the tests.

E.g. if we rename run_action_without to something else in setfrozentag_action_spec, we get:

Failures:

  1) Fastlane::Actions::SetfrozentagAction initialize GitHub Token is properly passed to the client properly passes the environment variable `GITHUB_TOKEN` to Octokit::Client
     Failure/Error: run_action_without(:github_token)

     NoMethodError:
       undefined method `run_action_without' for #<RSpec::ExampleGroups::FastlaneActionsSetfrozentagAction::Initialize::GitHubTokenIsProperlyPassedToTheClient:0x0000000109277748>
       Did you mean?  run_action_with
     Shared Example Group: "github_token_initialization" called from ./spec/setfrozentag_action_spec.rb:60
     ...

In Swift, we'd do something like constraining this to tests that conform to a certain protocol. There's no such thing in vanilla Ruby that I'm aware of.

There might be a way to update run_described_fastlane_action to add it the capability to do _with and _without, but that wouldn't solve the implicit dependency on default_params 🤔

At this point, the only thing I can think of is to document the requirement as a comment at the start of the file. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

shared examples can also contain metadata, that you can then pass as additional params when you call include_examples. I think that would be a better way to pass default_params.

As for _with and _without variants, I wonder if it's worth relying on those existing in the implementation of shared_examples vs just directly using default_params.except(…) and default_params.merge(…) so that we don't rely on those _with[out] methods existing

Copy link
Contributor Author

@raafaelima raafaelima Nov 10, 2022

Choose a reason for hiding this comment

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

shared examples can also contain metadata, that you can then pass as additional params when you call include_examples. I think that would be a better way to pass default_params.

Indeed, when I try to use the shared examples passing the default_params as metadata, the RSpec blocked me from doing so. The error that I receive is: As we have the default_params as a :let variable it is not available on an example group (e.g. a describe or context block). It is only available within individual examples (e.g. it blocks).

In that case, I don't know what is the best alternative here, if is to keep the shared examples relying on the existence of a variable named default_params and document this as @mokagio suggested. Or to redundant use another :let variable to pass that metadata as in this example 🤔

  describe 'initialize' do
    include_examples 'github_token_initialization' do
      let(:params) { default_params }
    end
  end

As for _with and _without variants, I wonder if it's worth relying on those existing in the implementation of shared_examples vs just directly using default_params.except(…) and default_params.merge(…) so that we don't rely on those _with[out] methods existing

Yeah, I have that "dilemma" when I was implementing this, I was first defaulting to those approaches of having the merge and except instead of relying on the _with and _without functions to exist. However, as I didn't find a good way to pass the params to inside the shared models, I end up following the same pattern that I was in the examples and relying on the existence of those methods as I do not want to duplicate them inside the shared examples.

Copy link
Contributor Author

@raafaelima raafaelima Nov 10, 2022

Choose a reason for hiding this comment

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

Also, there is this third option to do it, which is cleaner than the other one, but I find it a little bit confusing the way that it used the send with the variable name (from a perspective of a person who does not has so much knowledge of ruby 😅)

shared_examples 'github_token_initialization' do |params:|
  let(:params) { send(params) }
  ....
end

# spec/test.rb
describe 'initialize' do
 include_examples 'github_token_initialization', params: :default_params
end

But again, although I feel that this third option is cleaner, I have no strong option about which one is the right one that we should use here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tbh I'm not a fan of using send(…), both because it's lesser known, but also because it feels like a trick more than a proper official way to do this (the times I've used it was mostly to workaround some limitations rather than to implement something cleanly).

What about using metadata, but workaround the limitation that let is only visible in example and not groups… by declaring default_params as a function (def) rather than a let? I know that's what you had in an earlier iteration of the PR before I gave you some feedback that let is usually better suited for constant values, and that's still true, but if that is a blocker for us using it in include_examples' metadata, then I think it's ok to go back to def for that case then. At least I'd personally prefer this over using send 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I find the use of send kinda odd too 🙃.

However, even if I change default_params to a def function, I still receive the same error from the RSpec as using it as a let variable.

def default_params
{ 
  repository: 'test-repository',
  milestone: '10.1',
  github_token: 'Test-GithubToken-1234' 
}
end

#When I call
describe 'initialize' do
 include_examples 'github_token_initialization', default_params
end

# I receive the same error as the let
Failure/Error: include_examples 'github_token_initialization', default_params
  `default_params` is not available on an example group (e.g. a `describe` or `context` block). It is only available from within individual examples (e.g. `it` blocks) or from constructs that run in the scope of an example (e.g. `before`, `let`, etc).

The only way that seems to do the job (although is not a DRY solution), without using the send, Is to repeat the declaration of the parameters at the call of the shared_examples like this:

describe 'initialize' do
  include_examples 'github_token_initialization',  { repository: 'test-repository', milestone: '10.1', github_token: 'Test-GithubToken-1234' }
end

Let me know what you think about this suggestion 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge e.g. if it depends on another PR to be merged first, or if that PR is only to trigger CI on forks, …
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants