-
Notifications
You must be signed in to change notification settings - Fork 348
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
Fix: a bug that causes an error when pushing without setting git remote #396
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
f0464d4
test(e2e): add end-to-end tests for git push functionality
matscube 6063780
fix(commit.ts): add early return if OCO_GITPUSH is false to prevent u…
matscube abcb6b8
test(gitPush.test.ts): add test cases for OCO_GITPUSH environment var…
matscube 38ba663
docs(README.md): correct typo in the section about git push prompt co…
matscube 2057add
Merge branch 'dev' into fix/git-push-flow
matscube 0be27e2
test(e2e): update prompt text for git push confirmation
matscube File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,205 @@ | ||
import path from 'path'; | ||
import 'cli-testing-library/extend-expect'; | ||
import { exec } from 'child_process'; | ||
import { prepareTempDir } from './utils'; | ||
import { promisify } from 'util'; | ||
import { render } from 'cli-testing-library'; | ||
import { resolve } from 'path'; | ||
import { rm } from 'fs'; | ||
const fsExec = promisify(exec); | ||
const fsRemove = promisify(rm); | ||
|
||
/** | ||
* git remote -v | ||
* | ||
* [no remotes] | ||
*/ | ||
const prepareNoRemoteGitRepository = async (): Promise<{ | ||
gitDir: string; | ||
cleanup: () => Promise<void>; | ||
}> => { | ||
const tempDir = await prepareTempDir(); | ||
await fsExec('git init test', { cwd: tempDir }); | ||
const gitDir = path.resolve(tempDir, 'test'); | ||
|
||
const cleanup = async () => { | ||
return fsRemove(tempDir, { recursive: true }); | ||
}; | ||
return { | ||
gitDir, | ||
cleanup | ||
}; | ||
}; | ||
|
||
/** | ||
* git remote -v | ||
* | ||
* origin /tmp/remote.git (fetch) | ||
* origin /tmp/remote.git (push) | ||
*/ | ||
const prepareOneRemoteGitRepository = async (): Promise<{ | ||
gitDir: string; | ||
cleanup: () => Promise<void>; | ||
}> => { | ||
const tempDir = await prepareTempDir(); | ||
await fsExec('git init --bare remote.git', { cwd: tempDir }); | ||
await fsExec('git clone remote.git test', { cwd: tempDir }); | ||
const gitDir = path.resolve(tempDir, 'test'); | ||
|
||
const cleanup = async () => { | ||
return fsRemove(tempDir, { recursive: true }); | ||
}; | ||
return { | ||
gitDir, | ||
cleanup | ||
}; | ||
}; | ||
|
||
/** | ||
* git remote -v | ||
* | ||
* origin /tmp/remote.git (fetch) | ||
* origin /tmp/remote.git (push) | ||
* other ../remote2.git (fetch) | ||
* other ../remote2.git (push) | ||
*/ | ||
const prepareTwoRemotesGitRepository = async (): Promise<{ | ||
gitDir: string; | ||
cleanup: () => Promise<void>; | ||
}> => { | ||
const tempDir = await prepareTempDir(); | ||
await fsExec('git init --bare remote.git', { cwd: tempDir }); | ||
await fsExec('git init --bare other.git', { cwd: tempDir }); | ||
await fsExec('git clone remote.git test', { cwd: tempDir }); | ||
const gitDir = path.resolve(tempDir, 'test'); | ||
await fsExec('git remote add other ../other.git', { cwd: gitDir }); | ||
|
||
const cleanup = async () => { | ||
return fsRemove(tempDir, { recursive: true }); | ||
}; | ||
return { | ||
gitDir, | ||
cleanup | ||
}; | ||
}; | ||
|
||
describe('cli flow to push git branch', () => { | ||
it('do nothing when OCO_GITPUSH is set to false', async () => { | ||
const { gitDir, cleanup } = await prepareNoRemoteGitRepository(); | ||
|
||
await render('echo', [`'console.log("Hello World");' > index.ts`], { | ||
cwd: gitDir | ||
}); | ||
await render('git', ['add index.ts'], { cwd: gitDir }); | ||
|
||
const { queryByText, findByText, userEvent } = await render( | ||
`OCO_AI_PROVIDER='test' OCO_GITPUSH='false' node`, | ||
[resolve('./out/cli.cjs')], | ||
{ cwd: gitDir } | ||
); | ||
expect(await findByText('Confirm the commit message?')).toBeInTheConsole(); | ||
userEvent.keyboard('[Enter]'); | ||
|
||
expect( | ||
await queryByText('Choose a remote to push to') | ||
).not.toBeInTheConsole(); | ||
expect( | ||
await queryByText('Do you want to run `git push`?') | ||
).not.toBeInTheConsole(); | ||
expect( | ||
await queryByText('Successfully pushed all commits to origin') | ||
).not.toBeInTheConsole(); | ||
expect( | ||
await queryByText('Command failed with exit code 1') | ||
).not.toBeInTheConsole(); | ||
|
||
await cleanup(); | ||
}); | ||
|
||
it('push and cause error when there is no remote', async () => { | ||
const { gitDir, cleanup } = await prepareNoRemoteGitRepository(); | ||
|
||
await render('echo', [`'console.log("Hello World");' > index.ts`], { | ||
cwd: gitDir | ||
}); | ||
await render('git', ['add index.ts'], { cwd: gitDir }); | ||
|
||
const { queryByText, findByText, userEvent } = await render( | ||
`OCO_AI_PROVIDER='test' node`, | ||
[resolve('./out/cli.cjs')], | ||
{ cwd: gitDir } | ||
); | ||
expect(await findByText('Confirm the commit message?')).toBeInTheConsole(); | ||
userEvent.keyboard('[Enter]'); | ||
|
||
expect( | ||
await queryByText('Choose a remote to push to') | ||
).not.toBeInTheConsole(); | ||
expect( | ||
await queryByText('Do you want to run `git push`?') | ||
).not.toBeInTheConsole(); | ||
expect( | ||
await queryByText('Successfully pushed all commits to origin') | ||
).not.toBeInTheConsole(); | ||
|
||
expect( | ||
await findByText('Command failed with exit code 1') | ||
).toBeInTheConsole(); | ||
|
||
await cleanup(); | ||
}); | ||
|
||
it('push when one remote is set', async () => { | ||
const { gitDir, cleanup } = await prepareOneRemoteGitRepository(); | ||
|
||
await render('echo', [`'console.log("Hello World");' > index.ts`], { | ||
cwd: gitDir | ||
}); | ||
await render('git', ['add index.ts'], { cwd: gitDir }); | ||
|
||
const { findByText, userEvent } = await render( | ||
`OCO_AI_PROVIDER='test' node`, | ||
[resolve('./out/cli.cjs')], | ||
{ cwd: gitDir } | ||
); | ||
expect(await findByText('Confirm the commit message?')).toBeInTheConsole(); | ||
userEvent.keyboard('[Enter]'); | ||
|
||
expect( | ||
await findByText('Do you want to run `git push`?') | ||
).toBeInTheConsole(); | ||
userEvent.keyboard('[Enter]'); | ||
|
||
expect( | ||
await findByText('Successfully pushed all commits to origin') | ||
).toBeInTheConsole(); | ||
|
||
await cleanup(); | ||
}); | ||
|
||
it('push when two remotes are set', async () => { | ||
const { gitDir, cleanup } = await prepareTwoRemotesGitRepository(); | ||
|
||
await render('echo', [`'console.log("Hello World");' > index.ts`], { | ||
cwd: gitDir | ||
}); | ||
await render('git', ['add index.ts'], { cwd: gitDir }); | ||
|
||
const { findByText, userEvent } = await render( | ||
`OCO_AI_PROVIDER='test' node`, | ||
[resolve('./out/cli.cjs')], | ||
{ cwd: gitDir } | ||
); | ||
expect(await findByText('Confirm the commit message?')).toBeInTheConsole(); | ||
userEvent.keyboard('[Enter]'); | ||
|
||
expect(await findByText('Choose a remote to push to')).toBeInTheConsole(); | ||
userEvent.keyboard('[Enter]'); | ||
|
||
expect( | ||
await findByText('Successfully pushed all commits to origin') | ||
).toBeInTheConsole(); | ||
|
||
await cleanup(); | ||
}); | ||
}); |
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
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.
i now get what you mean, but i would keep this lines, especially the
if (stdout) outro(stdout);
log, because otherwise user won't see whypush
is not working bc of the silent return on line100
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.
@di-sukharev
If it's an intentional behavior to log an error when no remotes are set, I understand it.
I'll leave it as it is without fixing it.
I don't think the E2E tests related to
git push
will be wasted, so I'm thinking of converting this PR to a PR for E2E tests only. Is that okay?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.
awesome idea, lets have the tests 100%