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

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

Merged
merged 10 commits into from
Feb 1, 2022
1 change: 1 addition & 0 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ RSpec/FilePath:
- 'spec/android_localize_helper_spec.rb'
- 'spec/android_merge_translators_strings_spec.rb'
- 'spec/android_version_helper_spec.rb'
- 'spec/an_localize_libs_action_spec.rb'
- 'spec/an_metadata_update_helper_spec.rb'
- 'spec/an_update_metadata_source_spec.rb'
- 'spec/configuration_spec.rb'
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ _None_

* Ensure that the `gem push` step only runs on CI if lint, test and danger steps passed before it. [#325]
* Rename internal `Ios::L10nHelper` to `Ios::L10nLinterHelper`. [#328]
* Provide new `run_described_fastlane_action` to run Fastlane actions more thoroughly in unit tests [#330]

## 2.3.0

Expand Down
54 changes: 54 additions & 0 deletions spec/an_localize_libs_action_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
require 'spec_helper'

describe Fastlane::Actions::AnLocalizeLibsAction do
# This test is more of a way of ensuring `run_described_fastlane_action` handles array
# of hashes properly than a comprehensive test for the
# `an_localize_libs_action` action.
#
# Please consider expanding this test if you'll find yourself working on its
# action.
it 'merges the strings from the given array into the given main strings file' do
in_tmp_dir do |tmp_dir|
app_strings_path = File.join(tmp_dir, 'app.xml')
File.write(app_strings_path, android_xml_with_content('<string name="a_string">test from app</string>'))

lib1_strings_path = File.join(tmp_dir, 'lib1.xml')
File.write(lib1_strings_path, android_xml_with_content('<string name="a_lib1_string">test from lib 1</string>'))

lib2_strings_path = File.join(tmp_dir, 'lib2.xml')
File.write(lib2_strings_path, android_xml_with_content('<string name="a_lib2_string">test from lib 2</string>'))

run_described_fastlane_action(
app_strings_path: app_strings_path,
libs_strings_path: [
{ library: 'lib_1', strings_path: lib1_strings_path, exclusions: [] },
{ library: 'lib_2', strings_path: lib2_strings_path, exclusions: [] },
]
)

# 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
Comment on lines +29 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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…

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

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

I opened #332 to hopefully make the test clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

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…

expect(File.read(app_strings_path)).to eq(android_xml_with_content(expected))
end
end
end

def android_xml_with_content(content)
# 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.
Comment on lines +43 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

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 🙃

indented_content = content.chomp.lines.map { |l| " #{l}" }.join

return <<~XML
<?xml version="1.0" encoding="UTF-8"?>
<resources xmlns:tools="http://schemas.android.com/tools">
#{indented_content}
</resources>
XML
end
2 changes: 1 addition & 1 deletion spec/android_merge_translators_strings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

image

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

expect(@amtsTestUtils.read_result_data(test_script)).to eq(test_script['result']['content'])
end

Expand Down
6 changes: 3 additions & 3 deletions spec/ios_lint_localizations_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
expect(FileUtils).to receive(:cp_r)
expect_shell_command("#{install_dir}/bin/swiftgen", 'config', 'run', '--config', anything)

Fastlane::Actions::IosLintLocalizationsAction.run(
run_described_fastlane_action(
install_path: install_dir,
input_dir: empty_dataset,
base_lang: 'en'
Expand All @@ -43,7 +43,7 @@
# Second run: ensure we only run SwiftGen directly, without a call to curl nor unzip beforehand
expect_shell_command("#{install_dir}/bin/swiftgen", 'config', 'run', '--config', anything)

Fastlane::Actions::IosLintLocalizationsAction.run(
run_described_fastlane_action(
install_path: install_dir,
input_dir: empty_dataset,
base_lang: 'en'
Expand Down Expand Up @@ -103,7 +103,7 @@ def run_l10n_linter_test(data_file)
# remove this after the test ends, so that further executions of the test run faster.
# Only the first execution of the tests might take longer if it needs to install SwiftGen first to be able to run the tests.
install_dir = "vendor/swiftgen/#{Fastlane::Helper::Ios::L10nLinterHelper::SWIFTGEN_VERSION}"
result = Fastlane::Actions::IosLintLocalizationsAction.run(
result = run_described_fastlane_action(
install_path: install_dir,
input_dir: @test_data_dir,
base_lang: 'en'
Expand Down
64 changes: 31 additions & 33 deletions spec/ios_merge_strings_files_spec.rb
Original file line number Diff line number Diff line change
@@ -1,41 +1,39 @@
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

let(:test_data_dir) { File.join(File.dirname(__FILE__), 'test-data', 'translations', 'ios_l10n_helper') }

def fixture(name)
File.join(test_data_dir, name)
end

describe '#ios_merge_strings_files' do
it 'calls the action with the proper parameters and warn and return duplicate keys' do
# Arrange
messages = []
allow(FastlaneCore::UI).to receive(:important) do |message|
messages.append(message)
end
inputs = ['Localizable-utf16.strings', 'non-latin-utf16.strings']
def fixture(name)
File.join(test_data_dir, name)
end

Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmpdir|
inputs.each { |f| FileUtils.cp(fixture(f), tmpdir) }
describe '#ios_merge_strings_files' do
it 'calls the action with the proper parameters and warn and return duplicate keys' do
# Arrange
messages = []
allow(FastlaneCore::UI).to receive(:important) do |message|
messages.append(message)
end
inputs = ['Localizable-utf16.strings', 'non-latin-utf16.strings']

# Act
result = Dir.chdir(tmpdir) do
described_class.new.parse("lane :test do
ios_merge_strings_files(
paths: ['#{inputs[0]}', '#{inputs[1]}'],
destination: 'output.strings'
)
end").runner.execute(:test)
end
Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmpdir|
inputs.each { |f| FileUtils.cp(fixture(f), tmpdir) }

# Assert
expect(File).to exist(File.join(tmpdir, 'output.strings'))
expect(result).to eq(%w[key1 key2])
expect(messages).to eq([
'Duplicate key found while merging the `.strings` files: `key1`',
'Duplicate key found while merging the `.strings` files: `key2`',
])
# Act
result = Dir.chdir(tmpdir) do
run_described_fastlane_action(
paths: [inputs[0], inputs[1]],
destination: 'output.strings'
)
end

# Assert
expect(File).to exist(File.join(tmpdir, 'output.strings'))
expect(result).to eq(%w[key1 key2])
expect(messages).to eq(
[
'Duplicate key found while merging the `.strings` files: `key1`',
'Duplicate key found while merging the `.strings` files: `key2`',
]
)
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion spec/ios_merge_translators_strings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
def imts_run_test(script)
test_script = @imtsTestUtils.get_test_from_file(script)
@imtsTestUtils.create_test_data(test_script)
Fastlane::Actions::IosMergeTranslatorsStringsAction.run(strings_folder: @imtsTestUtils.test_folder_path)
run_described_fastlane_action(strings_folder: @imtsTestUtils.test_folder_path)
expect(@imtsTestUtils.read_result_data(test_script)).to eq(test_script['result']['content'])
end

Expand Down
10 changes: 6 additions & 4 deletions spec/shared_examples_for_update_metadata_source_action.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@
file_2_path = File.join(dir, '2.txt')
File.write(file_2_path, 'value 2')

described_class.run(
run_described_fastlane_action(
po_file_path: output_path,
release_version: '1.0',
source_files: {
key1: file_1_path,
key2: file_2_path
Expand Down Expand Up @@ -56,7 +57,7 @@
whats_new_path = File.join(dir, 'whats_new.txt')
File.write(whats_new_path, "- something new\n- something else new")

described_class.run(
run_described_fastlane_action(
po_file_path: output_path,
release_version: '1.23',
source_files: {
Expand Down Expand Up @@ -94,8 +95,9 @@
file_2_path = File.join(dir, '2.txt')
File.write(file_2_path, 'value 2')

described_class.run(
run_described_fastlane_action(
po_file_path: output_path,
release_version: '1.0',
source_files: {
key1: file_1_path,
key2: file_2_path
Expand Down Expand Up @@ -135,7 +137,7 @@
release_notes_path = File.join(dir, 'release_notes.txt')
File.write(release_notes_path, "- release notes\n- more release notes")

described_class.run(
run_described_fastlane_action(
po_file_path: output_path,
release_version: '1.23',
source_files: {
Expand Down
18 changes: 18 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,24 @@ def expect_shell_command(*command, exitstatus: 0, output: '')
expect(Open3).to receive(:popen2e).with(*command).and_yield(mock_input, mock_output, mock_thread)
end

# If the `described_class` of a spec is a `Fastlane::Action` subclass, it runs
# it with the given parameters.
#
def run_described_fastlane_action(parameters)
raise "Only call `#{__callee__}` from a spec describing a `Fastlane::Action` subclass." unless Fastlane::Actions.is_class_action?(described_class)

# Avoid logging messages about deprecated actions while running tests on them
allow(Fastlane::Actions).to receive(:is_deprecated?).and_return(false)
lane = <<~LANE
lane :test do
#{described_class.action_name}(
#{parameters.inspect}
)
end
LANE
Fastlane::FastFile.new.parse(lane).runner.execute(:test)
end

# Executes the given block within an ad hoc temporary directory.
def in_tmp_dir
Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmpdir|
Expand Down