-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[No QA] Update GitUtils to work with manual version bumps #44123
Conversation
@hayata-suenaga 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] |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
fc588bd
to
798ab1a
Compare
@@ -0,0 +1,473 @@ | |||
/** | |||
* @jest-environment node |
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.
Ported this from a shell script to Node.js because it made it easier to debug, and generally more flexible. Overriding console.log
to get the output of a script (as we were doing before) made it so that we couldn't really see logs from the script without breaking the assertions we had.
It was a fair amount of work (obviously, the test is large), but it makes it considerably more flexible to work with. You can easily set breakpoints at any step of the script, checkout the directory manually, run whatever commands you want, and continue.
@@ -96,7 +98,7 @@ module.exports = { | |||
plugins: ['@typescript-eslint', 'jsdoc', 'you-dont-need-lodash-underscore', 'react-native-a11y', 'react', 'testing-library'], | |||
parser: '@typescript-eslint/parser', | |||
parserOptions: { | |||
project: './tsconfig.json', | |||
project: path.resolve(__dirname, './tsconfig.json'), |
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.
This change was really just a QOL improvement to fix this issue with JetBrains IDE's.
inline lint warnings were not working for any files with a separate eslint config (tests, GitHub Actions, etc...)
This fixes that by using an absolute path for the project's tsconfig.json
..
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
[No QA] Update GitUtils to work with manual version bumps (cherry picked from commit 14210a6)
Manually CP PR #44123 to staging 🍒
🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 9.0.0-7 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.0.0-9 🚀
|
@roryabraham the file |
Hey, @roryabraham today I wanted to run tests and I got bizarre stuff going on. Undeterministic number of tests is failing. Almost all of them with the error visible on the screenshot. It stopped when I removed the file |
Sorry for the inconvenience. I think the flakiness from this test was fixed in #44967 |
Details
We include
--shallow-exclude
as an optimization to make our workflows faster (by avoiding fetching the full git history every time we checkout or fetch). This saves a couple minutes for every workflow run that needs git history (and this time would only go up over time).It broke because it relied on normal semver version bumps to work, but we manually jumped from 1.4 to 9.0. We were grabbing the previous patch version (8.0 in this case), and then running
git fetch origin main staging --no-tags --shallow-exclude="8.0.0-0"
. But because there was no tag for8.0.0-0
, that command failed.I fixed this by getting the previous version in a loop until we find one for which a tag actually exists.
Fixed Issues
$ https://expensify.slack.com/archives/C03V9A4TB/p1718917022181759
Tests
Merge and CP to test. I also added an automated test to cover this edge case. Did TDD to verify that it was failing as expected without the changes here, then made the changes and verified it passed ✅.
Offline tests
None.
QA Steps
None.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop