@iBotPeaches is there anything you would want changed here?

Added a regression test in trainer/spec/test_parser_spec.rb that directly exercises summaries_to_data with output_remove_retry_attempts: true for the fail-then-skip scenario. It asserts that only the final skipped row is kept and that all counts (failures, skips, retries) are correctly adjusted.

@iBotPeaches Is there anything you'd like changing?

Code wise? No. I got lost in time trying to just learn about the change. In my head I was trying to think how some folks run tests in order to find flake, so running with retries and encountering a failure is a still a failure.

Then I thought I was overthinking if it the retry mode was like Jest and its purpose is to workaround flaky tests, then I was going to see if this was opt-in via the parameter - then I forgot all about it.

Code wise? No. I got lost in time trying to just learn about the change. In my head I was trying to think how some folks run tests in order to find flake, so running with retries and encountering a failure is a still a failure.

Then I thought I was overthinking if it the retry mode was like Jest and its purpose is to workaround flaky tests, then I was going to see if this was opt-in via the parameter - then I forgot all about it.

I think for the case of people running to find a flake, they wouldn't be skipping a test. I'll admit it's a bit of a odd usecase but I think treating skip the same as a pass makes sense in this case. Appreciate it's hard to know what behaviours people are relying on. My hunch is this is sensible enough to cater for the expected behaviour. We only see this in rare cases where our tests are setup to "skip" if they hit a flaky Apple API, but might actually fail before that.

if you'd prefer this as an opt in parameter, just me know. Happy to make any changes

Reading through the options more - I believe this is a safe fix. You are just removing the retries if the test is skipped, which still semantically means "remove retry attempts"

        FastlaneCore::ConfigItem.new(key: :output_remove_retry_attempts,
                                     env_name: "TRAINER_OUTPUT_REMOVE_RETRY_ATTEMPTS",
                                     description: "Doesn't include retry attempts in the output",
                                     is_string: false,
                                     default_value: false)

Reading through the options more - I believe this is a safe fix. You are just removing the retries if the test is skipped, which still semantically means "remove retry attempts"

        FastlaneCore::ConfigItem.new(key: :output_remove_retry_attempts,
                                     env_name: "TRAINER_OUTPUT_REMOVE_RETRY_ATTEMPTS",
                                     description: "Doesn't include retry attempts in the output",
                                     is_string: false,
                                     default_value: false)

Yeah agreed, I think this is going to be expected behaviour though appreciate you can't garunatee how people are using it.