[scan] Forward force_legacy_xcresulttool to trainer#30092
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad4b1f81e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| type: Boolean, | ||
| default_value: false), | ||
| FastlaneCore::ConfigItem.new(key: :force_legacy_xcresulttool, | ||
| env_name: "SCAN_FORCE_LEGACY_XCRESULTTOOL", |
There was a problem hiding this comment.
When scan is invoked in CI with the trainer env var from the reported workaround (TRAINER_FORCE_LEGACY_XCRESULTTOOL=true bundle exec fastlane scan), this new option still resolves to its default false because Scan::Options only reads SCAN_FORCE_LEGACY_XCRESULTTOOL; Runner then passes that false value in a plain hash, so trainer's own env-backed configuration is never consulted. Include the trainer env name here (or otherwise merge it before calling Trainer::TestParser) so existing trainer-style configuration actually takes effect through scan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
After reading the PR description I was actually immediately thinking about this very thing — i.e. you mention in the PR description that you use SCAN_FORCE_LEGACY_XCRESULTTOOL for the env var but I immediately thought that for consistency with trainer, using TRAINER_FORCE_LEGACY_XCRESULTTOOL would probably make more sense (that way, if you set TRAINER_FORCE_LEGACY_XCRESULTTOOL env var it applies to both)?
| env_name: "SCAN_FORCE_LEGACY_XCRESULTTOOL", | |
| env_name: 'TRAINER_FORCE_LEGACY_XCRESULTTOOL', |
Alternatively, you could at least use both (ConfigItem supports env_names (plural) in addition to env_name to allow you to provide a list, the first one matching being the one taken into account at runtime
| env_name: "SCAN_FORCE_LEGACY_XCRESULTTOOL", | |
| env_names: ['SCAN_FORCE_LEGACY_XCRESULTTOOL', 'TRAINER_FORCE_LEGACY_XCRESULTTOOL'], |
There was a problem hiding this comment.
Went with env_names so both the scan name and TRAINER_FORCE_LEGACY_XCRESULTTOOL resolve. It also lines up with output_remove_retry_attempts right above, which forwards to trainer the same way.
|
Hmm is Codex from you? I don't see it configured on our side and its messaging suggests it is. |
|
That's an automatic reviewer tied to my account, not anything configured on fastlane's side. It kicked in when I opened the PR. I've turned it off so it won't post here again, sorry for the noise. |
|
No worries. The messaging just confused me as it read like it was configured on our side. |
|
I think @AliSoftware might be best to review this one. It looks like #30092 discussions |
AliSoftware
left a comment
AliSoftware
left a comment
There was a problem hiding this comment.
Thanks for fixing this, this was indeed an oversight!
The PR looks good so approving on principle. I just left a suggestion (actually agreeing with your Codex bot's review) about the env var name, that I think could be worth adjusting (using env_names to support both might be the best compromise?)
eyupcanakman
force-pushed
the
fix/scan-forward-force-legacy-xcresulttool
branch
from
ad4b1f8 to
3f9afa1
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @eyupcanakman 👋
Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a month, and your PR will be included in the next one.
Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀"
There was a problem hiding this comment.
Congratulations! 🎉 This was released as part of fastlane 2.237.0 🚀
github-actions
Bot
added
status: released
and removed
status: included-in-next-release
labels
3 participants
Motivation and Context
Resolves #29614.
scanruns its test results throughTrainer::TestParser.auto_convert, but it never forwarded theforce_legacy_xcresulttooloption that trainer added in #29463. Setting it (orTRAINER_FORCE_LEGACY_XCRESULTTOOL) had no effect when running tests throughscan, and the issue confirms it was left out by mistake.Description
Adds a
force_legacy_xcresulttooloption toscan(envSCAN_FORCE_LEGACY_XCRESULTTOOL, defaultfalse) and passes it through to trainer in theparamshash, next to the existingoutput_remove_retry_attempts.Testing Steps
bundle exec rspec scan/spec/runner_spec.rbbundle exec rubocop scan/lib/scan/options.rb scan/lib/scan/runner.rb scan/spec/runner_spec.rbChecklist
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