Refactor posting comment after testBuild action#13833
Merged
mountiny merged 16 commits intoJan 18, 2023
Merged
Conversation
melvin-bot
Bot
requested review from
francoisl
and removed request for
a team
|
@francoisl Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
francoisl
reviewed
Dec 27, 2022
|
|
||
| const androidQRCode = androidSuccess | ||
| ? `` | ||
| : "The code can't be generated, because the android build failed"; |
Contributor
There was a problem hiding this comment.
(Not a blocker) To make it super clear, I'd have specified "The QR code can't [...]"
Comment on lines
+12
to
+17
| const androidLink = androidSuccess | ||
| ? core.getInput('ANDROID_LINK') | ||
| : '❌ FAILED ❌'; | ||
| const desktopLink = desktopSuccess | ||
| ? core.getInput('DESKTOP_LINK') | ||
| : '❌ FAILED ❌'; |
Contributor
There was a problem hiding this comment.
Let's use the same style every where for consistency. It's still easy to read on one line, IMO, but feel free to change iOSLink and webLink on different lines if you prefer instead.
Suggested change
| const androidLink = androidSuccess | |
| ? core.getInput('ANDROID_LINK') | |
| : '❌ FAILED ❌'; | |
| const desktopLink = desktopSuccess | |
| ? core.getInput('DESKTOP_LINK') | |
| : '❌ FAILED ❌'; | |
| const androidLink = androidSuccess ? core.getInput('ANDROID_LINK') : '❌ FAILED ❌'; | |
| const desktopLink = desktopSuccess ? core.getInput('DESKTOP_LINK') : '❌ FAILED ❌'; |
| runs-on: ubuntu-latest | ||
| needs: validateActor | ||
| if: ${{ fromJSON(needs.validateActor.outputs.IS_TEAM_MEMBER) }} | ||
| if: ${{ needs.validateActor.outputs.READY_TO_BUILD == 'true' }} |
Contributor
There was a problem hiding this comment.
Out of curiosity, why not use fromJSON here?
Contributor
There was a problem hiding this comment.
Good call @francoisl, using fromJSON is our preferred style
Contributor
|
Initial review done, /cc @AndrewGable for familiarity. |
roryabraham
requested changes
Jan 3, 2023
| @@ -0,0 +1,70 @@ | |||
| const core = require('@actions/core'); | |||
Contributor
There was a problem hiding this comment.
It would be great to see some automated tests created for this JS action, to give us some additional confidence as reviewers, but also more importantly to make it easier to adjust the comment or code in the future.
Contributor
Author
There was a problem hiding this comment.
I will take care of that next week, it was very busy time and I was not able to pick it up...
| id: set_var | ||
| - name: Read JSONs with android paths | ||
| id: get_android_path | ||
| if: ${{ needs.android.result == 'success'}} |
Contributor
There was a problem hiding this comment.
Suggested change
| if: ${{ needs.android.result == 'success'}} | |
| if: ${{ needs.android.result == 'success' }} |
|
|
||
| - name: Read JSONs with iOS paths | ||
| id: get_ios_path | ||
| if: ${{ needs.ios.result == 'success'}} |
Contributor
There was a problem hiding this comment.
Suggested change
| if: ${{ needs.ios.result == 'success'}} | |
| if: ${{ needs.ios.result == 'success' }} |
| WEB: ${{ needs.web.result }} | ||
| ANDROID_LINK: ${{fromJson(steps.get_android_path.outputs.android_paths).html_path}} | ||
| DESKTOP_LINK: "https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/$PULL_REQUEST_NUMBER/NewExpensify.dmg" | ||
| IOS_LINK: ${{fromJson(steps.get_ios_path.outputs.ios_paths).html_path}} |
Contributor
There was a problem hiding this comment.
Suggested change
| IOS_LINK: ${{fromJson(steps.get_ios_path.outputs.ios_paths).html_path}} | |
| IOS_LINK: ${{ fromJson(steps.get_ios_path.outputs.ios_paths).html_path }} |
| if: ${{ fromJSON(needs.validateActor.outputs.READY_TO_BUILD) }} | ||
| uses: Expensify/App/.github/actions/javascript/postTestBuildComment@main | ||
| with: | ||
| PR_NUMBER: $PULL_REQUEST_NUMBER |
Contributor
There was a problem hiding this comment.
NAB but I think it's a bit clearer to explicitly reference the env context so it's more clear where this variable comes from:
Suggested change
| PR_NUMBER: $PULL_REQUEST_NUMBER | |
| PR_NUMBER: ${{ env.PULL_REQUEST_NUMBER }} |
Contributor
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
roryabraham
requested changes
Jan 10, 2023
Contributor
There was a problem hiding this comment.
Interesting pattern I haven't seen before. I'm not strongly opposed because this looks like a pretty clean approach. It would be nice to have some guidance for when we should use these "partial mocks". Also trying to understand what the alternative would look like. Would it be something like this?
mockGetInput.mockImplementation((input) => {
switch(input) {
case 'PR_NUMBER':
return 'success';
case 'IOS':
return 'success';
...
...
default:
throw new Error(`No value for ${input}`);
}
})
Contributor
There was a problem hiding this comment.
AFAIK that is the alternative.
Partial mock is useful when we expect method to be invoked several times in one test with different parameters and do not want to rely of order of those executions.
Contributor
There was a problem hiding this comment.
I think that this is straightforward enough to read that I won't push back on using when here
Contributor
There was a problem hiding this comment.
NAB but maybe a multiline template string would be more readable
staszekscp
force-pushed
the
ci/refactor-posting-comment
branch
from
6ae923b to
d3aef8a
Compare
Contributor
|
I have read the CLA Document and I hereby sign the CLA |
Contributor
Reviewer ChecklistNo screenshots provided, this is changing only Github actions
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
Contributor
|
@staszekscp Seems like the Validate Github actions is failling https://github.com/Expensify/App/actions/runs/3901654051/jobs/6672309108 Can you confirm that is fine? |
mountiny
reviewed
Jan 17, 2023
mountiny
left a comment
mountiny
left a comment
Contributor
There was a problem hiding this comment.
@staszekscp Left couple of comments!
Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>
mountiny
previously approved these changes
Jan 17, 2023
mountiny
left a comment
mountiny
left a comment
Contributor
There was a problem hiding this comment.
Thanks @mczernek Just gonna wait a bit for Rory, otherwise I would merge
@francoisl would you like to review?
mczernek
force-pushed
the
ci/refactor-posting-comment
branch
from
ac61d38 to
042bb0a
Compare
Contributor
|
@mczernek Sorry to have been a bottleneck here. We have some tests failing on main that have since been fixed. Can you please:
Thanks! |
roryabraham
previously approved these changes
Jan 17, 2023
roryabraham
left a comment
roryabraham
left a comment
Contributor
There was a problem hiding this comment.
No other major concerns from me except the ones listed above that are needed to get this PR in a mergable state. Approving so that once the above is done this PR can proceed without me.
AndrewGable
previously approved these changes
Jan 17, 2023
francoisl
previously approved these changes
Jan 17, 2023
| * @returns {String} | ||
| */ | ||
| function getTestBuildMessage() { | ||
| console.log('Input for android', core.getInput('ANDROID', {required: true})); |
Contributor
There was a problem hiding this comment.
(Not a blocker) - I'm curious why we only log android here and not the other platforms
Contributor
|
@mountiny feel free to merge whenever you're ready (and #13833 (comment) is done) since you also did the checklist. |
mczernek
dismissed stale reviews from francoisl, AndrewGable, and roryabraham
via
4ee57b7
mountiny
approved these changes
Jan 18, 2023
mountiny
left a comment
mountiny
left a comment
Contributor
There was a problem hiding this comment.
@staszekscp @mczernek Thank you guys for seeing this through!
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Contributor
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
13 tasks
Contributor
Contributor
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.2.56-0 🚀
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
This PR adds a JS action to post comment after
testBuildworkflow even if the build fails. It includes changes from #13726. An important thing to mention is that it could not be properly tested locally, because it needs multiple secrets to run correctly.Fixed Issues
#13784
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android