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

Skip failure on task to install XCTestHTMLReport and only publish result to artifact when the task succeeded #2620

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

KangxuanYe
Copy link
Contributor

@KangxuanYe KangxuanYe commented Nov 14, 2024

Description

As title says, this PR is to skip failure on task to install XCTestHTMLReport and only publish result to artifact when the task succeeded, considering we encounter the error as below every day.

'/System/Volumes/Preboot/Cryptexes/OS/Users/runner/work/_temp/codeql3000/distribution/codeql/tools/osx64/libtrace.dylib' (no such file), '/Users/runner/work/_temp/codeql3000/distribution/codeql/tools/osx64/libtrace.dylib' (fat file, but missing compatible architecture (have 'x86_64,arm64', need 'arm64e'))
dyld[7806]: tried: '/Users/runner/work/_temp/codeql3000/distribution/codeql/tools/osx64/libtrace.dylib' (fat file, but missing compatible architecture (have 'x86_64,arm64', need 'arm64e')), 
'/System/Volumes/Preboot/Cryptexes/OS/Users/runner/work/_temp/codeql3000/distribution/codeql/tools/osx64/libtrace.dylib' (no such file), '/Users/runner/work/_temp/codeql3000/distribution/codeql/tools/osx64/libtrace.dylib' (fat file, but missing compatible architecture (have 'x86_64,arm64', need 'arm64e'))
/Users/runner/work/_temp/939bbdf4-66ff-4572-8b7e-3fdb96cc1197.sh: line 1:  7806 Abort trap: 6           brew install xctesthtmlreport

Main changes in the PR: YML file in iOS

Validation

Hopefully it works when next time we encounter the same issue.

Unit Tests added: No

End-to-end tests added: No

Additional Requirements

Change file added: No

@KangxuanYe KangxuanYe marked this pull request as ready for review November 14, 2024 01:32
@KangxuanYe KangxuanYe requested a review from a team as a code owner November 14, 2024 01:32
@KangxuanYe KangxuanYe changed the title Update Skip failure on task to install XCTestHTMLReport and only publish result to artifact when the task succeeded Nov 14, 2024
@@ -18,7 +18,10 @@ steps:
targetType: inline
script: |
brew install xctesthtmlreport
Copy link
Contributor

Choose a reason for hiding this comment

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

If this fails (I assume this is the command that is failing intermittently?) is there a way to get more verbose logs about what the failure is?

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 they typically hide logs for us when we install this. Basically the error log it throws to console is the detailed one. Not sure if we can get more detailed log on VM.

@AE-MS AE-MS requested a review from maglims November 14, 2024 16:59
@@ -148,7 +151,7 @@ steps:
echo "ls -lR ${output}"
ls -lR "${output}"
displayName: Preparations for publishing results
condition: always()
condition: eq(variables['xctestHtmlReportInstalled'], 'Yes')
Copy link
Contributor

Choose a reason for hiding this comment

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

What effect will this have? Will the test results not be published if installation failed? Does that mean that any test failures will be overlooked if the html reporter didn't get installed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This condition is to say, only when xctestHtmlReportInstall task successfully be executed, this task will then be executed to publish results to the artifact.

Any test failures will NOT be overlooked. It just affects devs to figure out why test cases fail on CI pipeline with detailed screenshots and logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment explaining that? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

just wanted to make sure we are on the same page:

This does look like improvement, i.e, if we don't have xctestHtmlReport tool installed correctly, we shouldn't try to use the tool in later phase. The PR is not fixing existing environment issue, but rather than a pipeline task improvement. is my understanding correct ? @KangxuanYe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, 1000 out of 100 @maglims

@AE-MS I'll add comment to explain the change over tasks.

Copy link
Contributor

@AE-MS AE-MS left a comment

Choose a reason for hiding this comment

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

🕐

Copy link
Contributor Author

@KangxuanYe KangxuanYe left a comment

Choose a reason for hiding this comment

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

Replied to questions.

@@ -18,7 +18,10 @@ steps:
targetType: inline
script: |
brew install xctesthtmlreport

echo "##vso[task.setvariable variable=xctestHtmlReportInstalled]Yes"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you wonder what it does, please check this link: https://learn.microsoft.com/en-us/azure/devops/pipelines/process/conditions?view=azure-devops#variables-created-in-a-step-used-in-subsequent-step-conditions

Example:

steps:

# This step creates a new pipeline variable: doThing. This variable is available to subsequent steps.
- bash: |
    echo "##vso[task.setvariable variable=doThing]Yes"
  displayName: Step 1

# This step is able to use doThing, so it uses doThing in its condition
- script: |
    # Access the variable from Step 1 as an environment variable.
    echo "Value of doThing (as DOTHING env var): $DOTHING."
  displayName: Step 2
  condition: and(succeeded(), eq(variables['doThing'], 'Yes')) # or and(succeeded(), eq(variables.doThing, 'Yes'))

@@ -18,7 +18,10 @@ steps:
targetType: inline
script: |
brew install xctesthtmlreport
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 they typically hide logs for us when we install this. Basically the error log it throws to console is the detailed one. Not sure if we can get more detailed log on VM.

@@ -148,7 +151,7 @@ steps:
echo "ls -lR ${output}"
ls -lR "${output}"
displayName: Preparations for publishing results
condition: always()
condition: eq(variables['xctestHtmlReportInstalled'], 'Yes')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This condition is to say, only when xctestHtmlReportInstall task successfully be executed, this task will then be executed to publish results to the artifact.

Any test failures will NOT be overlooked. It just affects devs to figure out why test cases fail on CI pipeline with detailed screenshots and logs.

@@ -18,7 +18,10 @@ steps:
targetType: inline
script: |
brew install xctesthtmlreport

echo "##vso[task.setvariable variable=xctestHtmlReportInstalled]Yes"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment or two explaining what this does and why it is here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants