Switch to running `an_` and `gp_` metadata actions via Fastfile parsing #330

mokagio · 2022-01-28T13:54:12Z

Built on top of @AliSoftware's experiment to test actions by running them via a just-in-time Fastfile as opposed to via a run call to add an extra level of checks for the ConfigItems parsing from #326 (review).

This PR aims to extracts the boilerplate into a shared helper method to make calling the action in the tests straightforward.

mokagio · 2022-01-28T15:54:15Z

spec/ios_merge_strings_files_spec.rb

-describe Fastlane do
-  describe Fastlane::FastFile do
-    let(:test_data_dir) { File.join(File.dirname(__FILE__), 'test-data', 'translations', 'ios_l10n_helper') }
+describe Fastlane::Actions::IosMergeStringsFilesAction do


With the new run_described_action helper, we get to use the actual type we're testing here which I think makes the test a tiny bit clearer.

mokagio · 2022-01-28T15:56:07Z

spec/shared_examples_for_update_metadata_source_action.rb

-      described_class.run(
+      run_described_action(
        po_file_path: output_path,
+        release_version: '1.0',


@AliSoftware case in point for using this approach over run. It highlighted the fact that release_version is a required ConfigItem, even though not passing it to the run call doesn't result in an error 👍

Ah! Interesting!

As a side note (and I think we have this tracked somewhere, just not sure to remember where) one of the improvements we might want to implement on the replacement for those actions is to auto-detect the release_version an stop having to pass it to the actions. Given the action should already have everything it needs to guess/determine the current release version (either by reading it from the xcconfig file or by the release/* branch name), it would be nice to remove the need for it during your future refactoring 🙂

mokagio · 2022-01-28T15:57:06Z

spec/spec_helper.rb

+# it with the given parameters.
+#
+def run_described_action(parameters, lane_name = 'test')
+  expect(described_class).to be < Fastlane::Action, "Only call `#{__callee__}` from a spec describing a `Fastlane::Action` subclass."


I went for a test failure over a raise so that we'd get this error as part of the test run result. What do you think?

I'm fine either way, though I'd have used raise because if called from the wrong place in the test code itself, that's a programmer error from whoever write the spec/test, as opposed to a correct test which goes red and fail an expectation because the SUT / implementation it's testing changed behavior.

To be a failed expectation suggests that the SUT was expected to behave in a certain way, and might have been for a while (making the test green for a while) but suddenly we change something in the implementation code and the test goes red because our expectations of how the SUT is supposed to behave were invalidated. So it's a behavior error and a notification that the behavior changed (after some refactoring of the implementation maybe).

By contrast, calling this method from a test which does not have an Action subclass as its SUT is a programmer error in the way you wrote the test, not an expectation of the SUT itself risking changing, so to me a raise would make more sense here. But to be honest, that's also not that big a deal 🤷

mokagio · 2022-01-28T15:59:43Z

spec/spec_helper.rb

+    # rubocop:disable Style/CaseLikeIf
+    if value.is_a?(Hash)
+      "#{key}: {\n#{stringify_for_fastlane(value)}\n}"
+    elsif value.is_a?(String)
+      "#{key}: \"#{value}\""
+    else
+      "#{key}: #{value}"
+    end
+    # rubocop:enable Style/CaseLikeIf


I tried to use case-when, but I couldn't find a neat way to put value in the case call, because I need to call is_a? on it. The only thing I could do was:

case when value.is_a?(Hash) ... when ... ... else ... end

Which raised a different lint error saying I should have used if-elsif instead 🙃 ♻️

Keen to know if there's a different way to write this.

case value when Hash puts 'Hash!' when String puts 'String!' else puts "What's that?" end

AliSoftware

I LOVE the idea of that helper ❤️

Unfortunately, the stringify_for_fastlane method has a potential for having a lot of issues and edge cases, so we need to work deeper on a solution for doing what this method is doing… And this might end up being quite tricky to be sure to cover all cases…

Worth testing if Ruby's built-in .inspect wouldn't be enough after all to format that like we need, and cover all cases?

In any case, don't forget to test that with action parameters that are a bit more convoluted in their structure (but that we unfortunately still have cases for right now), like parameters containing arrays of strings or even arrays of hashes (cf. an_localize_libs_action.rb amongst others)

PS: I also didn't check if Fastlane themselves didn't have a helper method on their own that would do something similar 🤔

AliSoftware · 2022-01-28T16:18:15Z

spec/spec_helper.rb

+# If the `described_class` of a spec is a `Fastlane::Action` subclass, it runs
+# it with the given parameters.
+#
+def run_described_action(parameters, lane_name = 'test')


Suggested change

def run_described_action(parameters, lane_name = 'test')

def run_described_action(parameters, lane_name: 'test')

I'd suggest using a named parameters here especially when the previous arguments parameters is of variable length (in that case typically an implicit Hash, making it read like if it was a variadic number of named parameters)

Though even with a named parameter I'm curious how the call site would look like if you wanted to provided a different lane name. I think the only option at call site would be to make the Hash of parameters explicitly wrapped in { … } in that case, to avoid having … lane_nane: 'foo' be considered part of the parameters Hash otherwise.

All that being said, I wonder what's the point of allowing to customize the lane name here, given it's only used as a dummy value to run it once parsed… so I think you can even totally remove it after all.

Suggested change

def run_described_action(parameters, lane_name = 'test')

def run_described_action(parameters)

spec/spec_helper.rb

mokagio · 2022-02-01T11:50:41Z

Thanks @AliSoftware. I went for that basic version of the method mostly because it's the first option that worked for me. I figured if we had issues down the line, we'd deal with them then.

In any case, don't forget to test that with action parameters that are a bit more convoluted in their structure (but that we unfortunately still have cases for right now), like parameters containing arrays of strings or even arrays of hashes (cf. an_localize_libs_action.rb amongst others)

I didn't think of array of hashes, but the arrays of string case was already covered by adopting the new method in ios_merge_strings_files_spec.rb 😄

Worth testing if Ruby's built-in .inspect wouldn't be enough after all to format that like we need, and cover all cases?

I just did that, thank you for the suggestion. All the existing tests still pass, which is a great place to start 😄

I'll keep working on this as my focus for the day, given it's soft-blocking you, as mentioned in #331

This should be more robust and require no extra maintenance on our end. See discussion at #330 (review)

The main reason for this is to verify that the new implementation of the method to run an action in the tests from f3bdab3 works with an `Array` of `Hash`.

@AliSoftware

This is for extra clarity. Thanks @AliSoftware, see: #330 (comment)

This is for both consistency and to ensure the implementation can handle different input parameters scenarios.

mokagio · 2022-02-01T16:24:02Z

spec/android_merge_translators_strings_spec.rb

@@ -35,7 +35,7 @@
 def amts_run_test(script)
  test_script = @amtsTestUtils.get_test_from_file(script)
  @amtsTestUtils.create_test_data(test_script)
-  Fastlane::Actions::AndroidMergeTranslatorsStringsAction.run(strings_folder: @amtsTestUtils.test_folder_path)
+  run_described_fastlane_action(strings_folder: @amtsTestUtils.test_folder_path)


Going through Fastlane to run the action can add considerable noise in the test output because the action's log now appear in it:

I'm sure there's a way to disable this, but I haven't found it yet 😞

This looks like the output of UI.deprecated calls, so maybe just add allow(FastlaneCore::UI).to receive(:deprecated) around the helper would be enough to catch and mute them?

mokagio · 2022-02-01T16:29:16Z

@AliSoftware , I should have addressed your suggestions with my latest round of changes.

I haven't looked into whether Fastlane has code for this and I also haven't done any extra testing for the #inspect approach. I think that, if we run in some weird edge case, we can either look into dealing with it then or fallback to calling #run on the < Action itself.

What I did, though, was making sure the implementation worked for all existing usages. That's obviously a subset of the possible cases, but I'm okay with it because it's the one we care about right now, and we're not trying to share that helper method as a catch all solution to the wider community :simple_smile:

Let me know what you think when you have the time 👍 🙇‍♂️

@mokagio

cc @mokagio

AliSoftware · 2022-02-01T18:21:28Z

Going through Fastlane to run the action can add considerable noise in the test output because the action's log now appear in it:

@mokagio Took the liberty to push 8e96443 to fix this myself directly, by stubbing is_deprecated? to return false when testing actions.

AliSoftware · 2022-02-01T18:23:48Z

I think that, if we run in some weird edge case, we can either look into dealing with it then or fallback to calling #run on the < Action itself.

Totally agree, it's already a great starting point and covers all our use cases anyway, so I'm all good with that approach 👍

AliSoftware

Looks mostly good, thanks for providing this, this will nicely clean stuff up 👍 👏

Note that I've taken the liberty to push a change to the run_described_fastlane_action to mute all those deprecation logs that you were seeing when running the tests. (I also took the occasion to switch the guard statement from an expect to a raise, as suggested here, and use a build-in Fastlane helper for the check); I hope you don't mind.

Was also surprised by the indentation glitch you commented about in the XML stuff… quite curious about this.

Anyway, I'm only leaving this review as a comment and not a formal approval to leave you a chance to add the missing entry in the CHANGELOG.md without auto-merge kicking in beforehand 😛 ; but once you've added the entry, this should be all good to merge 👍

AliSoftware · 2022-02-01T17:00:15Z

spec/an_localize_libs_action_spec.rb

+  # I couldn't find a way to interpolate a multiline string preserving its
+  # indentation in the heredoc below, so I hacked the following transformation
+  # of the input that adds the desired indentation to all lines.


👍

FYI iinm you can add post-processing methods to the heredoc prelude, e.g. expected = <<~XML.gsub(/^/,' ') and similar if you need such thing. Haven't used that in ages but I believe that's the syntax.

That being said, that wouldn't really help here because the only thing you want to indent is the content, but not the surrounding <resources …> tags in the rest of that heredoc… so I think the way you did it is fine and probably the cleanest we can get given the context 🙃

AliSoftware · 2022-02-01T17:35:31Z

spec/an_localize_libs_action_spec.rb

+      # Notice the extra indentation in the library strings. The action doesn't
+      # modify the app's strings content indentation, but it applies its own
+      # standard to the values read from the given library strings
+      expected = <<~XML
+        <string name="a_string">test from app</string>
+          <string name="a_lib1_string">test from lib 1</string>
+          <string name="a_lib2_string">test from lib 2</string>
+      XML


Wait, does that mean that in practice, the merged XML is expected to contain different indentations, i.e. a certain indentation for strings coming from the app_strings_path but a different indentation for strings coming from libraries?

Never noticed this discrepency in practice 🙀 (and that surprises me given iirc, we merge the XMLs using Nokogiri, which re-formats the XML when it saves it on disk, so I'd expect it to normalize the indentation? 🤷🤔

Besides, I don't see such inconsistent indendation in https://github.com/wordpress-mobile/WordPress-Android/blob/trunk/fastlane/resources/values/strings.xml so I'm not sure what to make of this test and why we'd have a different indentation in that test but not in real life in the client project?

I think that's simply due to the indentation in the test using 2 spaces, as it's the default style for this codebase, and the action indenting XML using 4 spaces.

I found that interesting and left it there. If it makes for a confusing test, maybe we could update the examples to all use 4 spaces and leave a note about it being the "default"?

I did a quick search and apparently there is no recommended indentation level for XML. The spec mentions white space handling:

In editing XML documents, it is often convenient to use "white space" (spaces, tabs, and blank lines) to set apart the markup for greater readability. Such white space is typically not intended for inclusion in the delivered version of the document. On the other hand, "significant" white space that should be preserved in the delivered version is common, for example in poetry and source code.

I'm guessing because most indentation is only meant for readability, then it's up to each author (or formatter author) to choose what they find appropriate.

Ah so this would in practice be an artefact of the tests and how the content used for those XML tests are inlined and assembled in the specs themselves (as opposed to if you were using fixture files) then? While in real use case in client apps there's no such indentation discrepancy between the tags from the app and the ones from the libs?

If that's so then yeah better rewrite the tests to make this less confusing and more importantly make it match reality then 😅

Ah wait yeah actually I just realized why that is. This is because the first line of your content already gets 2 extra spaces from the heredoc itself! Which is why you have to remove 2 spaces from the first line of your content here to compensate.

So this is just misleading and there is in fact no indentation difference in the XML itself.

You could make it less confusing by adding the 2 spaces on the first line here (to match the rest of the lines), and removing the 2 extra leading spaces in your helper method that includes it in the heresoc block.

Mmmh actually re-checked your heredoc and the 2 leasing spaces I thought were there aren't 🤨 so scratch what I just said. Still, pretty strange and worth looking into…

I took a bit of digging, but I discovered the difference in indentation to be due to the code adding lines from the XML adding 4 spaces. See:

release-toolkit/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb

Line 67 in da405f2

main_strings.xpath('//string').last().add_next_sibling("\n#{' ' * 4}#{string_line.to_xml().strip}")

I opened #332 to hopefully make the test clearer.

Yeah I remember that our code hardcodes 4 spaces like that, what I found unclear is not why we'd have 4 spaces indentation (that I expected) but instead why the first line With have a different indentation than the rest of the lines.

Your other PR is clearer, thanks for opening it, and it actually does match exactly what I expected the test (and expected XML) would look like from the start so that change in #332 definitively makes more sense to me.

That being said I'm still puzzled as to how the code in this present PR even passed before in its current state (without #332) then. Because the change you made in #332 consists of removing 2 spaces from all the lines of what content looked like except the first line, while in the code where you later inject that content into the XML heredoc you added 2 spaces (switching from 2 to 4 leading spaces on each line) to all the lines. So the 2 extra spaces in indented_content balance the 2 spaces removed on content just like I expect… except for that first line of content which gets 2 extra space from the map but didn't get 2 spaces removed from content to balance.

If I'm not missing anything from ☝️ (but I'm sure I do otherwise that doesn't make sense) that would mean that the string you use for the expectation changed between this PR and #332 (gaining 2 leading spaces only on the first line, as the rest of the lines balance out) yet your specs were green both before and after that change on the first line of the expectation string?!

I think I need some rest, I must be missing something obvious…

mokagio · 2022-02-01T20:07:17Z

Note that I've taken the liberty to push a change to the run_described_fastlane_action to mute all those deprecation logs that you were seeing when running the tests.

Thank you for that. Interestingly, you ended up using allow(Fastlane::Actions).to receive(:is_deprecated?).and_return(false) which is pretty cool.

Did you by any chance try the other approaches like allow(Fastlane::UI).to receive(:deprecated)? I did try a bunch of those and couldn't get rid of the messages.

(I also took the occasion to switch the guard statement from an expect to a raise, as suggested here, and use a build-in Fastlane helper for the check); I hope you don't mind.

That's okay 👍 Your rationale is solid, it was my expectation for what raise would have done was incorrect. I was expecting something like in iOS where the whole suite crashes, but instead RSpec reports it as just another failure:

Also, using is_class_action? is pretty neat.

Anyway, I'm only leaving this review as a comment and not a formal approval to leave you a chance to add the missing entry in the CHANGELOG.md without auto-merge kicking in beforehand 😛

Thank you for thinking of it, too. I didn't think this was worth an entry, but you obviously do and it's easy to do.

See #330 (review)

mokagio · 2022-02-01T20:15:06Z

I've dismissed the change request as per the following review:

Anyway, I'm only leaving this review as a comment and not a formal approval to leave you a chance to add the missing entry in the CHANGELOG.md without auto-merge kicking in beforehand 😛 ; but once you've added the entry, this should be all good to merge 👍

Having added the entry in 8448e02, I'm going to admin merge as soon as CI is green.

AliSoftware · 2022-02-02T11:06:36Z

Did you by any chance try the other approaches like allow(Fastlane::UI).to receive(:deprecated)? I did try a bunch of those and couldn't get rid of the messages.

I tried at first but when I saw that it wasn't working, I looked at fastlane's source code to understand when and how those messages were printed, which is what led me to this solution instead 😉

That's okay 👍 Your rationale is solid, it was my expectation for what raise would have done was incorrect. I was expecting something like in iOS where the whole suite crashes, but instead RSpec reports it as just another failure:

🙂 👌

Thank you for thinking of it, too. I didn't think this was worth an entry, but you obviously do and it's easy to do.

Eh actually I thought of it mostly because I saw the Peril comment about it and didn't think about it twice… but now that you mention it and I think about it, yeah it would have been totally ok to omit it (I just didn't even take 2s to ponder about that when I commented, and instead just reacted to Peril mindlessly there 😅 ). Eh, doesn't hurt to have one anyway 👍

AliSoftware · 2022-02-02T11:08:21Z

I've dismissed the change request as per the following review

Oh, sorry about that, I thought that by adding a new PR review as "comment" (and not "approval" nor "request changes") it would automatically replace my previous "request changes" review; I should have double-checked and dismissed it myself indeed!

mokagio added 4 commits January 28, 2022 15:51

Switch to running an_ and gp_ metadata actions via Fastfile parsing

18fbc8c

Move run_described_action method to spec_helper.rb

f141e7b

Use new run_describe_action in ios_merge_strings_files_spec.rb

f53fe80

Add expectation so run_describe_action is called only from actions

bb2768b

mokagio force-pushed the add-fastlane-action-runner-helper branch from d427773 to bb2768b Compare January 28, 2022 15:38

mokagio commented Jan 28, 2022

View reviewed changes

mokagio requested a review from AliSoftware January 28, 2022 16:02

mokagio marked this pull request as ready for review January 28, 2022 16:03

mokagio enabled auto-merge January 28, 2022 16:05

AliSoftware previously requested changes Jan 28, 2022

View reviewed changes

AliSoftware mentioned this pull request Jan 31, 2022

[L10n Tooling: i3] Re-implement download-from-glotpress logic from .py and .swift scripts #331

Merged

mokagio added 4 commits February 1, 2022 12:51

Replace custom method with #inspect call

f3bdab3

This should be more robust and require no extra maintenance on our end. See discussion at #330 (review)

Add one test to cover an_localize_libs_action

5cf9dd7

The main reason for this is to verify that the new implementation of the method to run an action in the tests from f3bdab3 works with an `Array` of `Hash`.

Rename run_described_action into run_described_fastlane_action

635f9ca

This is for extra clarity. Thanks @AliSoftware, see: #330 (comment)

Adopt new run_described_fastlane_action across all tests

135fdb1

This is for both consistency and to ensure the implementation can handle different input parameters scenarios.

mokagio commented Feb 1, 2022

View reviewed changes

mokagio requested a review from AliSoftware February 1, 2022 16:25

Improved run_described_fastlane_action to reduce logs during tests

8e96443

cc @mokagio

AliSoftware reviewed Feb 1, 2022

View reviewed changes

Add CHANGELOG.md entry for #330

8448e02

mokagio disabled auto-merge February 1, 2022 20:15

mokagio merged commit da405f2 into trunk Feb 1, 2022

mokagio deleted the add-fastlane-action-runner-helper branch February 1, 2022 20:16

mokagio mentioned this pull request Feb 1, 2022

Clarify expectation on XML indentation in AnLocalizeLibsAction spec #332

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to running `an_` and `gp_` metadata actions via Fastfile parsing #330

Switch to running `an_` and `gp_` metadata actions via Fastfile parsing #330

mokagio commented Jan 28, 2022 •

edited

Loading

mokagio Jan 28, 2022

mokagio Jan 28, 2022

AliSoftware Jan 28, 2022

mokagio Jan 28, 2022

AliSoftware Jan 28, 2022

mokagio Jan 28, 2022

AliSoftware Jan 28, 2022 •

edited

Loading

AliSoftware left a comment

AliSoftware Jan 28, 2022

mokagio commented Feb 1, 2022

mokagio Feb 1, 2022

AliSoftware Feb 1, 2022

mokagio commented Feb 1, 2022

AliSoftware commented Feb 1, 2022

AliSoftware commented Feb 1, 2022

AliSoftware left a comment •

edited

Loading

AliSoftware Feb 1, 2022

AliSoftware Feb 1, 2022

mokagio Feb 1, 2022

AliSoftware Feb 1, 2022

AliSoftware Feb 1, 2022

AliSoftware Feb 1, 2022

mokagio Feb 1, 2022

AliSoftware Feb 1, 2022

mokagio commented Feb 1, 2022

mokagio commented Feb 1, 2022

AliSoftware commented Feb 2, 2022

AliSoftware commented Feb 2, 2022

	def run_described_action(parameters, lane_name = 'test')
	def run_described_action(parameters, lane_name: 'test')

	def run_described_action(parameters, lane_name = 'test')
	def run_described_action(parameters)

Switch to running an_ and gp_ metadata actions via Fastfile parsing #330

Switch to running an_ and gp_ metadata actions via Fastfile parsing #330

Conversation

mokagio commented Jan 28, 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

AliSoftware Jan 28, 2022 • edited Loading

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

mokagio commented Feb 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio commented Feb 1, 2022

AliSoftware commented Feb 1, 2022

AliSoftware commented Feb 1, 2022

AliSoftware left a comment • 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

mokagio commented Feb 1, 2022

mokagio commented Feb 1, 2022

AliSoftware commented Feb 2, 2022

AliSoftware commented Feb 2, 2022

Switch to running `an_` and `gp_` metadata actions via Fastfile parsing #330

Switch to running `an_` and `gp_` metadata actions via Fastfile parsing #330

mokagio commented Jan 28, 2022 •

edited

Loading

AliSoftware Jan 28, 2022 •

edited

Loading

AliSoftware left a comment •

edited

Loading