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

chore: scripts should propagate errors #3197

Merged
merged 2 commits into from
Aug 3, 2023
Merged

chore: scripts should propagate errors #3197

merged 2 commits into from
Aug 3, 2023

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Aug 1, 2023

📜 Description

💡 Motivation and Context

In another PR, I've noticed some scripts don't error-out if there's a failure, e.g. this test build failure.
This makes it harder to find actual cause of a failure down the road.

Instead, scripts should fail when an error occurs - this is what this PR does. After changing the scripts, I've also had to make a few fixes that for the newly discovered issues.

However, there's one bigger issue with thread-sanitizer. It hasn't been running because it failed after a build due to malformed command. After fixing the call, the sanitizer fails. I suggest disabling the thread sanitizer step in CI for now and fixing its errors in a separate maintenance issue. See latest log: https://github.com/getsentry/sentry-cocoa/suites/14737489895/artifacts/837486751

#skip-changelog

💚 How did you test it?

Checked that there are no CI failures.

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #3197 (4a758cf) into main (3cba0e8) will decrease coverage by 0.008%.
The diff coverage is 100.000%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3197       +/-   ##
=============================================
- Coverage   89.204%   89.196%   -0.008%     
=============================================
  Files          502       502               
  Lines        54058     54046       -12     
  Branches     19405     19391       -14     
=============================================
- Hits         48222     48207       -15     
+ Misses        4986      4877      -109     
- Partials       850       962      +112     
Files Changed Coverage Δ
Tests/SentryTests/Helper/SentryDeviceTests.mm 82.352% <100.000%> (+0.210%) ⬆️

... and 26 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3cba0e8...4a758cf. Read the comment docs.

- 'scripts/ci-select-xcode.sh'
- 'scripts/xcode-test.sh'
- '.codecov.yml'
- "Sources/**"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry about the quotes - it's auto-formatted by vscode. I can roll this back manually if needed.

@vaind vaind marked this pull request as ready for review August 1, 2023 18:43
@vaind vaind mentioned this pull request Aug 2, 2023
7 tasks
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

scripts/tests-with-thread-sanitizer.sh Show resolved Hide resolved
@kahest kahest mentioned this pull request Aug 3, 2023
@vaind vaind merged commit 983de17 into main Aug 3, 2023
51 of 52 checks passed
@vaind vaind deleted the chore/errors-in-scripts branch August 3, 2023 12:52
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.

4 participants