Add Unit tests for milestone actions #425

raafaelima · 2022-11-03T15:04:56Z

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! 😄

spec/create_new_milestone_action_spec.rb

raafaelima · 2022-11-03T15:16:29Z

spec/setfrozentag_action_spec.rb

+    end
+  end
+
+  describe 'update_milestone' do


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

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.

…ne-actions

AliSoftware

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.

AliSoftware · 2022-11-03T18:11:08Z

spec/close_milestone_action_spec.rb

+
+    before do
+      ENV['GITHUB_TOKEN'] = nil
+      mock_params[:github_token] = nil


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.

AliSoftware · 2022-11-03T18:12:36Z

spec/close_milestone_action_spec.rb

+    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


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

AliSoftware · 2022-11-03T18:25:57Z

spec/close_milestone_action_spec.rb

+    end
+  end
+
+  describe 'get_milestone' do


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 🤔

AliSoftware · 2022-11-03T18:39:01Z

spec/close_milestone_action_spec.rb

+    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


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

AliSoftware · 2022-11-03T18:51:00Z

spec/create_new_milestone_action_spec.rb

+    end
+  end
+
+  describe 'get_last_milestone' do


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.

AliSoftware · 2022-11-03T19:00:35Z

spec/create_new_milestone_action_spec.rb

+    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


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.

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? 🤔

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 👍

AliSoftware · 2022-11-03T19:16:14Z

spec/setfrozentag_action_spec.rb

+    end
+  end
+
+  describe 'update_milestone' do


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

raafaelima · 2022-11-04T01:35:32Z

lib/fastlane/plugin/wpmreleasetoolkit/actions/common/create_new_milestone_action.rb

@@ -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,


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.

This is great! It's even a small step towards addressing #278 that we opened a while ago about this 🙂

lib/fastlane/plugin/wpmreleasetoolkit/actions/common/create_new_milestone_action.rb

raafaelima · 2022-11-04T01:42:21Z

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.

raafaelima · 2022-11-04T01:52:54Z

spec/setfrozentag_action_spec.rb

+  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


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?

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

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

lib/fastlane/plugin/wpmreleasetoolkit/actions/common/create_new_milestone_action.rb

AliSoftware · 2022-11-04T16:52:56Z

spec/close_milestone_action_spec.rb

-    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


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

AliSoftware · 2022-11-04T16:53:17Z

spec/close_milestone_action_spec.rb

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


AliSoftware · 2022-11-04T17:11:35Z

spec/close_milestone_action_spec.rb

    end
  end
+
+  def run_action_without_key(key)


nit:

Suggested change

def run_action_without_key(key)

def run_action_without(key:)

AliSoftware · 2022-11-04T17:13:37Z

spec/close_milestone_action_spec.rb

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


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

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

AliSoftware · 2022-11-04T18:23:23Z

spec/create_new_milestone_action_spec.rb

+  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


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?

Yeah! It will be more organized 😄

AliSoftware · 2022-11-04T18:26:29Z

spec/create_new_milestone_action_spec.rb

+        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


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…

AliSoftware · 2022-11-04T18:32:57Z

spec/create_new_milestone_action_spec.rb

+        expect(github_helper).to receive(:create_milestone).with(
+          anything,
+          anything,
+          anything,
+          anything,
+          default_code_freeze_days,
+          anything
+        )


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.

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 😄

Update: This is addressed on PR #426 😄

AliSoftware · 2022-11-04T18:33:47Z

spec/create_new_milestone_action_spec.rb

+  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


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

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 🤔

AliSoftware · 2022-11-04T18:34:30Z

spec/setfrozentag_action_spec.rb

+  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


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

To DRY the tests that required the token, as they will validate the same cases against the presence of the Token

AliSoftware

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.

AliSoftware · 2022-11-07T11:40:13Z

spec/setfrozentag_action_spec.rb

+  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


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.

mokagio · 2022-11-10T13:14:55Z

spec/shared_examples_for_actions_with_github_token.rb

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


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?

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

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.

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.

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 🙃

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 😄

raafaelima added 3 commits November 3, 2022 14:52

CloseMilestoneAction: Add UnitTests

c37f29e

CreateNewMilestoneAction: Add UnitTests

6dfd767

SetFrozenTagAction: Add UnitTests

22b9d44

raafaelima commented Nov 3, 2022

View reviewed changes

spec/create_new_milestone_action_spec.rb Outdated Show resolved Hide resolved

raafaelima commented Nov 3, 2022

View reviewed changes

spec/create_new_milestone_action_spec.rb Outdated Show resolved Hide resolved

raafaelima commented Nov 3, 2022

View reviewed changes

AliSoftware requested review from AliSoftware and mokagio November 3, 2022 15:23

raafaelima added 2 commits November 3, 2022 18:32

SetFrozenTagSpec: Fix Failing Tests due the introduce the Hash-splat …

96acb93

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

Merge branch 'wordpress-mobile:trunk' into add/unit-tests-for-milesto…

f695b04

…ne-actions

AliSoftware reviewed Nov 3, 2022

View reviewed changes

raafaelima added 3 commits November 4, 2022 01:32

CloseMilestoneAction: Fix Issues Pointed on PR

3472f9e

SetFrozenTagAction: Fix Issues Pointed on PR

cdd96c2

CreateNewMilestoneAction: Fix Issues Pointed on PR

3a5a240

raafaelima commented Nov 4, 2022

View reviewed changes

lib/fastlane/plugin/wpmreleasetoolkit/actions/common/create_new_milestone_action.rb Show resolved Hide resolved

raafaelima commented Nov 4, 2022

View reviewed changes

raafaelima requested review from AliSoftware and removed request for mokagio November 4, 2022 01:58

AliSoftware reviewed Nov 4, 2022

View reviewed changes

AliSoftware requested a review from mokagio November 4, 2022 18:35

raafaelima added 4 commits November 6, 2022 14:40

Specs: Add Shared Examples for Actions that require Github_Token

5e656dd

To DRY the tests that required the token, as they will validate the same cases against the presence of the Token

CloseMilestoneAction: Add Shared Examples of GithubToken

b6de4cd

CreateMilestoneAction: Add Shared Examples of GithubToken

9eb95c2

SetFrozenTagAction: Add Shared Examples of GithubToken

87ae1d4

raafaelima mentioned this pull request Nov 6, 2022

Add named params and yard docs to create_milestone method at GithubHelper #426

Merged

AliSoftware previously approved these changes Nov 7, 2022

View reviewed changes

AliSoftware self-assigned this Nov 7, 2022

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

mokagio reviewed Nov 10, 2022

View reviewed changes

spencertransier mentioned this pull request Apr 26, 2023

Replace all instances of is_string with type #469

Merged

4 tasks

	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

Add Unit tests for milestone actions #425

Are you sure you want to change the base?

Add Unit tests for milestone actions #425

Conversation

raafaelima commented Nov 3, 2022 • edited Loading

What does this solve?

What does PR #420 was about?

Any Dependencies?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raafaelima Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raafaelima Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raafaelima commented Nov 4, 2022

raafaelima Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

AliSoftware Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raafaelima Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

raafaelima Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raafaelima commented Nov 3, 2022 •

edited

Loading

raafaelima Nov 4, 2022 •

edited

Loading

raafaelima Nov 4, 2022 •

edited

Loading

raafaelima Nov 4, 2022 •

edited

Loading

AliSoftware Nov 4, 2022 •

edited

Loading

raafaelima Nov 10, 2022 •

edited

Loading

raafaelima Nov 10, 2022 •

edited

Loading