Skip to content
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

Add test for clasp versions #145

Merged
merged 3 commits into from Apr 26, 2018
Merged

Add test for clasp versions #145

merged 3 commits into from Apr 26, 2018

Conversation

campionfellin
Copy link
Collaborator

Signed-off-by: campionfellin campionfellin@gmail.com

  • npm run test succeeds.
  • npm run lint succeeds.
  • Appropriate changes to README are included in PR.

Signed-off-by: campionfellin <campionfellin@gmail.com>
@coveralls
Copy link

coveralls commented Apr 25, 2018

Pull Request Test Coverage Report for Build 119

  • 0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
  • 131 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.5%) to 11.703%

Files with Coverage Reduction New Missed Lines %
tests/test.js 29 41.82%
index.js 102 6.92%
Totals Coverage Status
Change from base Build 112: 0.5%
Covered Lines: 116
Relevant Lines: 745

💛 - Coveralls

Signed-off-by: campionfellin <campionfellin@gmail.com>
tests/test.ts Outdated

describe.skip('Test getFileType function from utils', () => {
it('should return the lowercase file type correctly', () => {
const type = getFileType('SERVER_JS');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function only appears in index.ts once. We could probably inline it, as all it does is a toLowerCase unless the filetype is SERVER_JS, where it then just converts it to js

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is fine standalone. It's good to test it for coverage.

tests/test.ts Outdated
'clasp', ['versions'], { encoding: 'utf8' },
);
expect(result.stdout).to.contain('~ ');
expect(result.stdout).to.contain(' Versions ~');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem like a very strong test. Can we create a version, then ensure the version we created is the same as the version listed?

tests/test.ts Outdated
const url = getScriptURL('abcdefghijklmnopqrstuvwxyz');
expect(url).to.equal('https://script.google.com/d/abcdefghijklmnopqrstuvwxyz/edit');
const url2 = getScriptURL('1234567890abcdefg');
expect(url2).to.equal('https://script.google.com/d/1234567890abcdefg/edit');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This URL doesn't test anything different than the previous URL.
Omit it.

tests/test.ts Outdated
it('should return the lowercase file type correctly', () => {
const type = getFileType('SERVER_JS');
expect(type).to.equal('js');
const type2 = getFileType('GS');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline all consts in this function. They're only used once.

tests/test.ts Outdated

describe.skip('Test getFileType function from utils', () => {
it('should return the lowercase file type correctly', () => {
const type = getFileType('SERVER_JS');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is fine standalone. It's good to test it for coverage.

Copy link
Contributor

@grant grant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking as requesting changes.

Signed-off-by: campionfellin <campionfellin@gmail.com>
);
expect(result.stdout).to.contain('Created version ');
expect(result.status).to.equal(0);
versionNumber = result.stdout.substring(result.stdout.lastIndexOf(" "), result.stdout.length-2);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gets version number of version created

);
expect(result.stdout).to.contain('~ ');
expect(result.stdout).to.contain(' Versions ~');
expect(result.stdout).to.contain(versionNumber + ' - ');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And checks if that version is listed.

Copy link
Contributor

@grant grant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
The tslint comment can be addressed in a different PR.

);
expect(result.stdout).to.contain('Created version ');
expect(result.status).to.equal(0);
versionNumber = result.stdout.substring(result.stdout.lastIndexOf(" "), result.stdout.length-2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tslint should've checked for spaces aside the - operator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, use ' not ".

const result = spawnSync(
'clasp', ['versions'], { encoding: 'utf8' },
);
expect(result.stdout).to.contain('~ ');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally changing the ~ character (i.e. ) wouldn't break this test.
I suppose it's not that big of a deal if we change it later.

@grant grant merged commit 5cfa540 into google:master Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants