-
Notifications
You must be signed in to change notification settings - Fork 11
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
provide better access to test logs #132
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
==========================================
- Coverage 99.29% 99.28% -0.01%
==========================================
Files 8 8
Lines 564 563 -1
Branches 61 61
==========================================
- Hits 560 559 -1
Misses 4 4 ☔ View full report in Codecov by Sentry. |
a9648c2
to
0573811
Compare
Hi @thrix. Yes, it seems we stopped showing the tests log URL in our logs in v2. |
AS far as I know, it was present in Bash version. See here: testing-farm-as-github-action/action.yml Line 91 in 3e8634e
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I looked at code and did not hit any issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zmiklank, please also add tests for the new output so we do not cause regressions later.
For reference:
testing-farm-as-github-action/tests/action.test.ts
Lines 220 to 224 in 6d060a1
// Test outputs | |
expect(process.env['OUTPUT_REQUEST_ID']).toMatchInlineSnapshot('"1"'); | |
expect(process.env['OUTPUT_REQUEST_URL']).toMatchInlineSnapshot( | |
'"https://api.dev.testing-farm.io/requests/1"' | |
); |
Good catch, thanks. Added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Related: #130
@jamacku PTAL.