[scan][trainer] hotfix for xcresulttool spec changes >= XC16b3#22147
Merged
giginet merged 7 commits intofastlane:masterfrom Jul 24, 2024
Merged
[scan][trainer] hotfix for xcresulttool spec changes >= XC16b3#22147giginet merged 7 commits intofastlane:masterfrom
giginet merged 7 commits intofastlane:masterfrom
Conversation
giginet
reviewed
Jul 23, 2024
Collaborator
giginet
left a comment
There was a problem hiding this comment.
Thank you!
It's better to add specs on https://github.com/fastlane/fastlane/blob/master/trainer/spec/test_parser_spec.rb with stubbing
trainer/lib/trainer/test_parser.rb
Outdated
| result_bundle_object_raw = execute_cmd("xcrun xcresulttool get --format json --path #{path}") | ||
| # Hotfix: From Xcode 16 beta 3 'xcresulttool get --format json' has been deprecated; '--legacy' flag required to keep on using the command | ||
| xcresulttool_cmd = "xcrun xcresulttool get --format json --path #{path}" | ||
| xcresulttool_cmd << ' --legacy' if `xcrun --version`.gsub('xcrun version ', '').to_f >= 70.0 # Version of xcrun bundled in Xcode 16 beta 3 |
Collaborator
There was a problem hiding this comment.
Could you use xcrun xcresulttool version instead to check backward compatibility?
This behavior depends on xcresulttool version, not xcrun version.
xcresulttool version 23021, format version 3.53 (current)
format version can be used.
It's better to extract the method to parse xcresulttool version
private
def xcresulttool_version
result = `xcrun xcresulttool version`
match = result.match(/format version (?<format_version>\d+\.\d+)/)
match[:format_version]
end
Contributor
Author
There was a problem hiding this comment.
$ DEVELOPER_DIR=/Applications/Xcode_15.4.app xcrun xcresulttool version
xcresulttool version 22608, format version 3.49 (current)
$ DEVELOPER_DIR=/Applications/Xcode_16_beta.app xcrun xcresulttool version
xcresulttool version 23019.2, format version 3.53 (current)
$ DEVELOPER_DIR=/Applications/Xcode_16_beta_2.app xcrun xcresulttool version
xcresulttool version 23019.2, format version 3.53 (current)
$ DEVELOPER_DIR=/Applications/Xcode_16_beta_3.app xcrun xcresulttool version
xcresulttool version 23021, format version 3.53 (current)
Thanks for the information. I have never known there is xcresulttool version command and just accepted suggestion from other.
Pinpointing xcresulttool version would be the best answer. Thanks again!
trainer/lib/trainer/test_parser.rb
Outdated
| # Executes xcresulttool to get JSON format of the result bundle object | ||
| result_bundle_object_raw = execute_cmd("xcrun xcresulttool get --format json --path #{path}") | ||
| # Hotfix: From Xcode 16 beta 3 'xcresulttool get --format json' has been deprecated; '--legacy' flag required to keep on using the command | ||
| xcresulttool_cmd = "xcrun xcresulttool get --format json --path #{path}" |
Collaborator
There was a problem hiding this comment.
I prefer to use an array to construct args
args = %W(
xcrun
xcresulttool
get
--format
json
--path
#{path}
)
args << "--legacy" if xcresulttool_version >= 3.53
execute_cmd(args.join(' '))
Collaborator
|
I saw the spec implementation; writing tests for these changes is difficult, so we may skip writing specs. |
giginet
reviewed
Jul 23, 2024
Collaborator
giginet
left a comment
There was a problem hiding this comment.
Thank you for fixing the PR.
It almost seems to be good.
trainer/lib/trainer/test_parser.rb
Outdated
| ) | ||
|
|
||
| # Version of xcresulttool bundled in Xcode 16 beta 3 was 23021 | ||
| xcresulttool_version_string = `xcrun xcresulttool version`.split(',')[0] |
Collaborator
There was a problem hiding this comment.
Could you note the expected output from xcrun xcresulttool version on the comment?
# xcresulttool version 23021, format version 3.53 (current)
Collaborator
There was a problem hiding this comment.
I prefer to use regex and the named capture for code readability
match = result.match(/xcresulttool version (?<version>\d+)/)
version = match[:version]&.to_f
Comment on lines
+29
to
+56
| describe "#generate_cmd_parse_xcresult" do | ||
| it "appends '--legacy' to command for parse_xcresult on >= Xcode 16 beta 3", requires_xcode: true do | ||
| xcresult_sample_path = "./trainer/spec/fixtures/Test.test_result.xcresult" | ||
| @model = Trainer::TestParser.new(xcresult_sample_path) | ||
| allow_any_instance_of(Trainer::TestParser).to receive(:`).and_return("xcresulttool version 23021, format version 3.53 (current)") | ||
| actual = @model.send(:generate_cmd_parse_xcresult, xcresult_sample_path) | ||
| expect(actual).to eq("xcrun xcresulttool get --format json --path #{xcresult_sample_path} --legacy") | ||
| end | ||
|
|
||
| it "use traditional command for parse_xcresult on < Xcode 16 beta 3", requires_xcode: true do | ||
| xcresult_sample_path = "./trainer/spec/fixtures/Test.test_result.xcresult" | ||
| @model = Trainer::TestParser.new(xcresult_sample_path) | ||
| allow_any_instance_of(Trainer::TestParser).to receive(:`).and_return("xcresulttool version 22608, format version 3.49 (current)") | ||
| actual = @model.send(:generate_cmd_parse_xcresult, xcresult_sample_path) | ||
| expect(actual).to eq("xcrun xcresulttool get --format json --path #{xcresult_sample_path}") | ||
| end | ||
| end |
Collaborator
There was a problem hiding this comment.
It's better to unify test logic with context
Suggested change
| describe "#generate_cmd_parse_xcresult" do | |
| it "appends '--legacy' to command for parse_xcresult on >= Xcode 16 beta 3", requires_xcode: true do | |
| xcresult_sample_path = "./trainer/spec/fixtures/Test.test_result.xcresult" | |
| @model = Trainer::TestParser.new(xcresult_sample_path) | |
| allow_any_instance_of(Trainer::TestParser).to receive(:`).and_return("xcresulttool version 23021, format version 3.53 (current)") | |
| actual = @model.send(:generate_cmd_parse_xcresult, xcresult_sample_path) | |
| expect(actual).to eq("xcrun xcresulttool get --format json --path #{xcresult_sample_path} --legacy") | |
| end | |
| it "use traditional command for parse_xcresult on < Xcode 16 beta 3", requires_xcode: true do | |
| xcresult_sample_path = "./trainer/spec/fixtures/Test.test_result.xcresult" | |
| @model = Trainer::TestParser.new(xcresult_sample_path) | |
| allow_any_instance_of(Trainer::TestParser).to receive(:`).and_return("xcresulttool version 22608, format version 3.49 (current)") | |
| actual = @model.send(:generate_cmd_parse_xcresult, xcresult_sample_path) | |
| expect(actual).to eq("xcrun xcresulttool get --format json --path #{xcresult_sample_path}") | |
| end | |
| end | |
| describe "#generate_cmd_parse_xcresult" do | |
| let(:xcresult_sample_path) { "./trainer/spec/fixtures/Test.test_result.xcresult" } | |
| let(:subject) { Trainer::TestParser.new(xcresult_sample_path) } | |
| before do | |
| allow_any_instance_of(Trainer::TestParser).to receive(:`).and_return(version) | |
| end | |
| context 'with >= Xcode 16 beta 3' do | |
| let(:version) { 'xcresulttool version 23021, format version 3.53 (current)' } | |
| it 'should pass `--legacy`' do | |
| actual = subject.send(:generate_cmd_parse_xcresult, xcresult_sample_path) | |
| expect(actual).to eq("xcrun xcresulttool get --format json --path #{xcresult_sample_path} --legacy") | |
| end | |
| end | |
| context 'with < Xcode 16 beta 3' do | |
| let(:version) { 'xcresulttool version 22608, format version 3.49 (current)' } | |
| it 'should not pass `--legacy`' do | |
| actual = subject.send(:generate_cmd_parse_xcresult, xcresult_sample_path) | |
| expect(actual).to eq("xcrun xcresulttool get --format json --path #{xcresult_sample_path}") | |
| end | |
| end | |
| end |
Using context to describe the situation is the best practice of rspec.
Contributor
Author
There was a problem hiding this comment.
Thanks for the review again!
The only thing I concern is: if I accept the suggestion, allow_all_instance_of(Trainer::TestParser) will also affect execute_cmd, which also uses backtick.
It will result in error by executing version as a command.
Failure/Error: let(:subject) { Trainer::TestParser.new(xcresult_sample_path) }
JSON::ParserError:
809: unexpected token at 'xcresulttool version 23021, format version 3.53 (current)'
This might not be the best practice (or maybe anti-pattern), but initiating Trainer::TestParser.new as a instance variable in before block might be inevitable...
describe "#generate_cmd_parse_xcresult" do
let(:xcresult_sample_path) { "./trainer/spec/fixtures/Test.test_result.xcresult" }
before {
@subject = Trainer::TestParser.new(xcresult_sample_path)
allow_any_instance_of(Trainer::TestParser).to receive(:`).and_return(version)
}
...
(same as your suggestion other than using `@subject` for sending call to private methods)
Collaborator
There was a problem hiding this comment.
Sorry, my suggestion was wrong.
I confirmed that this snippet works well.
describe "#generate_cmd_parse_xcresult" do
let(:xcresult_sample_path) { "./trainer/spec/fixtures/Test.test_result.xcresult" }
let!(:subject) { Trainer::TestParser.new(xcresult_sample_path) }
let(:command) { subject.send(:generate_cmd_parse_xcresult, xcresult_sample_path) }
before do
allow(File).to receive(:expand_path).with(xcresult_sample_path).and_return(xcresult_sample_path)
allow_any_instance_of(Trainer::TestParser).to receive(:`).with('xcrun xcresulttool version').and_return(version)
end
context 'with >= Xcode 16 beta 3' do
let(:version) { 'xcresulttool version 23021, format version 3.53 (current)' }
let(:expected) { "xcrun xcresulttool get --format json --path #{xcresult_sample_path} --legacy" }
it 'should pass `--legacy`' do
expect(command).to eq(expected)
end
end
context 'with < Xcode 16 beta 3' do
let(:version) { 'xcresulttool version 22608, format version 3.49 (current)' }
let(:expected) { "xcrun xcresulttool get --format json --path #{xcresult_sample_path}" }
it 'should not pass `--legacy`' do
expect(command).to eq(expected)
end
end
endWe have to use let! to define subject before the stubbing.
My point is using context to follow the better rspec
Contributor
Author
Amended along with the review comments:
How I checked if the fixes work:I tried on my local machine using Xcode 15.4, Xcode 16 beta, and Xcode 16 beta 3.
And... would anyone please restart appveyor/pr to see if CI is green? I got no permission to start that job I think... |
giginet
approved these changes
Jul 24, 2024
Collaborator
giginet
left a comment
There was a problem hiding this comment.
Thank your continuous effort. Looks great for me 🎉
I'll resolve CI error and I'll merge this soon.
Thank you again!
|
|
||
| # e.g. DEVELOPER_DIR=/Applications/Xcode_16_beta_3.app | ||
| # xcresulttool version 23021, format version 3.53 (current) | ||
| match = `xcrun xcresulttool version`.match(/xcresulttool version (?<version>\d+),/) |
4d6738c to
142ca11
Compare
Collaborator
|
Great work! Thank you! |
Merged
6 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
bundle exec rspecfrom the root directory to see all new and existing tests passbundle exec rubocop -ato ensure the code style is validci/circlecibuilds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)Motivation and Context
Resolves #22132
From Xcode 16 beta 3,
fastlane ios scanstarted to fail after xctest had finished.The reason is: There has been a deprecation and stderr in traditional
xcrun xcresulttoolcommand usage.Error details of xcresulttool related portion
I tried new way when obtaining xcresult object (
xcresulttool get testsor something) but--idoption was not showing up in the reportFor these reasons, decided to add
--legacyif using Xcode 16 beta 3.It would take quite a time to use the new type of
xcresulttoollike in the deprecation notice.Description
used changes of this branch as a fastlane gem and tried
run_testsunder XC15 and 16.specs in trainer had nothing to test with this part so I tried scan on both Xcode 15.4 and 16 beta 3
Testing Steps
Both environments are in CI, I cloned this single branch and
bundle installto use bundle fastlane.scan with Xcode 16 beta 3 using this branch
scan with Xcode 15.4 using this branch
Seems no differences in the result!