-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
remove trailing comma from commands #263
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
import * as command from '../src/command' | ||
import * as os from 'os' | ||
|
||
/* eslint-disable @typescript-eslint/unbound-method */ | ||
|
||
let originalWriteFunction: (str: string) => boolean | ||
|
||
describe('@actions/core/src/command', () => { | ||
beforeAll(() => { | ||
originalWriteFunction = process.stdout.write | ||
}) | ||
|
||
beforeEach(() => { | ||
process.stdout.write = jest.fn() | ||
}) | ||
|
||
afterEach(() => {}) | ||
|
||
afterAll(() => { | ||
process.stdout.write = (originalWriteFunction as unknown) as ( | ||
str: string | ||
) => boolean | ||
}) | ||
|
||
it('command only', () => { | ||
command.issueCommand('some-command', {}, '') | ||
assertWriteCalls([`::some-command::${os.EOL}`]) | ||
}) | ||
|
||
it('command with message', () => { | ||
command.issueCommand('some-command', {}, 'some message') | ||
assertWriteCalls([`::some-command::some message${os.EOL}`]) | ||
}) | ||
|
||
it('command with message and properties', () => { | ||
command.issueCommand( | ||
'some-command', | ||
{prop1: 'value 1', prop2: 'value 2'}, | ||
'some message' | ||
) | ||
assertWriteCalls([ | ||
`::some-command prop1=value 1,prop2=value 2::some message${os.EOL}` | ||
]) | ||
}) | ||
|
||
it('command with one property', () => { | ||
command.issueCommand('some-command', {prop1: 'value 1'}, '') | ||
assertWriteCalls([`::some-command prop1=value 1::${os.EOL}`]) | ||
}) | ||
|
||
it('command with two properties', () => { | ||
command.issueCommand( | ||
'some-command', | ||
{prop1: 'value 1', prop2: 'value 2'}, | ||
'' | ||
) | ||
assertWriteCalls([`::some-command prop1=value 1,prop2=value 2::${os.EOL}`]) | ||
}) | ||
|
||
it('command with three properties', () => { | ||
command.issueCommand( | ||
'some-command', | ||
{prop1: 'value 1', prop2: 'value 2', prop3: 'value 3'}, | ||
'' | ||
) | ||
assertWriteCalls([ | ||
`::some-command prop1=value 1,prop2=value 2,prop3=value 3::${os.EOL}` | ||
]) | ||
}) | ||
}) | ||
|
||
// Assert that process.stdout.write calls called only with the given arguments. | ||
function assertWriteCalls(calls: string[]): void { | ||
expect(process.stdout.write).toHaveBeenCalledTimes(calls.length) | ||
|
||
for (let i = 0; i < calls.length; i++) { | ||
expect(process.stdout.write).toHaveBeenNthCalledWith(i + 1, calls[i]) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,21 +37,21 @@ describe('@actions/core', () => { | |
|
||
it('exportVariable produces the correct command and sets the env', () => { | ||
core.exportVariable('my var', 'var val') | ||
assertWriteCalls([`::set-env name=my var,::var val${os.EOL}`]) | ||
assertWriteCalls([`::set-env name=my var::var val${os.EOL}`]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe I missed it but do we have any tests that validate multiple properties (and thus does write the comma)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep command.test.ts is new and tests several combinations |
||
}) | ||
|
||
it('exportVariable escapes variable names', () => { | ||
core.exportVariable('special char var \r\n];', 'special val') | ||
expect(process.env['special char var \r\n];']).toBe('special val') | ||
assertWriteCalls([ | ||
`::set-env name=special char var %0D%0A%5D%3B,::special val${os.EOL}` | ||
`::set-env name=special char var %0D%0A%5D%3B::special val${os.EOL}` | ||
]) | ||
}) | ||
|
||
it('exportVariable escapes variable values', () => { | ||
core.exportVariable('my var2', 'var val\r\n') | ||
expect(process.env['my var2']).toBe('var val\r\n') | ||
assertWriteCalls([`::set-env name=my var2,::var val%0D%0A${os.EOL}`]) | ||
assertWriteCalls([`::set-env name=my var2::var val%0D%0A${os.EOL}`]) | ||
}) | ||
|
||
it('setSecret produces the correct command', () => { | ||
|
@@ -101,7 +101,7 @@ describe('@actions/core', () => { | |
|
||
it('setOutput produces the correct command', () => { | ||
core.setOutput('some output', 'some value') | ||
assertWriteCalls([`::set-output name=some output,::some value${os.EOL}`]) | ||
assertWriteCalls([`::set-output name=some output::some value${os.EOL}`]) | ||
}) | ||
|
||
it('setFailure sets the correct exit code and failure message', () => { | ||
|
@@ -171,7 +171,7 @@ describe('@actions/core', () => { | |
|
||
it('saveState produces the correct command', () => { | ||
core.saveState('state_1', 'some value') | ||
assertWriteCalls([`::save-state name=state_1,::some value${os.EOL}`]) | ||
assertWriteCalls([`::save-state name=state_1::some value${os.EOL}`]) | ||
}) | ||
|
||
it('getState gets wrapper action state', () => { | ||
|
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 is so beforeAll can reference
originalWriteFunction
. note,core.test.ts
does a similar thing so there was a precedent for disabling this already