Add more tests #158

campionfellin · 2018-05-02T16:32:07Z

A few notes:

Looks like we should make saveProjectId return a promise so we can use await syntax.

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

Also, noted that saveProjectId should be async or return a promise Signed-off-by: campionfellin <campionfellin@gmail.com>

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

campionfellin · 2018-05-02T16:32:54Z

tests/test.ts

+  it('should save the scriptId correctly', () => {
+    spawnSync('rm', ['.clasp.json']);
+    saveProjectId('12345');
+    setTimeout(() => {


This is here because otherwise, it won't have saved the projectId before running the test.

Don't set a timeout.
Change the definition of saveProjectId.

var isSaved = async saveProjectId('12345'); // async is only used for tests. In src, we don't to read back the value

Source (with async):

/** * Saves the script ID in the project dotfile. * @param {string} scriptId The script ID */ export async function saveProjectId(scriptId: string): void { return DOTFILE.PROJECT().write({ scriptId }); // Save the script id }

saveProjectId uses dotf .write(), which returns a Promise.

Not sure if that's the exact syntax, but something like that would be better than a 3000ms read file timeout.

Yeah I was just putting that there so the tests didn't fail. I'll go ahead and make another commit to this branch with an update of saveProjectId

campionfellin · 2018-05-02T16:33:26Z

tests/test.ts

+describe('Test saveProjectId function from utils', () => {
+  it('should save the scriptId correctly', () => {
+    spawnSync('rm', ['.clasp.json']);
+    saveProjectId('12345');


Should this be renamed to saveScriptId?

I believe so according to the API reference.
The API is not very clear what the difference between a scriptId and a projectId. We could probably just say scriptId.

I'll change it from saveProjectId to saveScriptId. In my opinion, we should make a function at sometime that would support clasp logs by allowing users to add their GCP project ID to the .clasp.json file without having to manually edit it.

coveralls · 2018-05-02T16:37:02Z

Pull Request Test Coverage Report for Build 146

0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
73 unchanged lines in 2 files lost coverage.
Overall coverage increased (+1.7%) to 13.968%

Files with Coverage Reduction	New Missed Lines	%
src/utils.js	3	18.64%
tests/test.js	70	32.88%

Totals
Change from base Build 141:	1.7%
Covered Lines:	157
Relevant Lines:	855

💛 - Coveralls

coveralls · 2018-05-02T16:37:02Z

Pull Request Test Coverage Report for Build 144

0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
22 unchanged lines in 1 file lost coverage.
Overall coverage increased (+2.5%) to 14.821%

Files with Coverage Reduction	New Missed Lines	%
tests/test.js	22	52.46%

Totals
Change from base Build 141:	2.5%
Covered Lines:	157
Relevant Lines:	817

💛 - Coveralls

coveralls · 2018-05-02T16:37:02Z

Pull Request Test Coverage Report for Build 144

0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
22 unchanged lines in 1 file lost coverage.
Overall coverage increased (+2.5%) to 14.821%

Files with Coverage Reduction	New Missed Lines	%
tests/test.js	22	52.46%

Totals
Change from base Build 141:	2.5%
Covered Lines:	157
Relevant Lines:	817

💛 - Coveralls

grant

LGTM once setTimeout is removed. Thanks!

grant · 2018-05-04T15:06:43Z

tests/test.ts

+describe('Test saveProjectId function from utils', () => {
+  it('should save the scriptId correctly', () => {
+    spawnSync('rm', ['.clasp.json']);
+    saveProjectId('12345');


I believe so according to the API reference.
The API is not very clear what the difference between a scriptId and a projectId. We could probably just say scriptId.

grant · 2018-05-04T15:08:10Z

tests/test.ts

  it('should return the lowercase file type correctly', () => {
    expect(getFileType('SERVER_JS')).to.equal('js');
    expect(getFileType('GS')).to.equal('gs');
    expect(getFileType('JS')).to.equal('js');
  });
 });

+describe('Test getAPIFileType function from utils', () => {
+  it('should return the uppercase file type correctly', () => {
+    expect(getAPIFileType('file.GS')).to.equal('SERVER_JS');


Add a few more cases: file.js, file.jsx, file.js.html.

grant · 2018-05-04T15:18:53Z

tests/test.ts

+  it('should save the scriptId correctly', () => {
+    spawnSync('rm', ['.clasp.json']);
+    saveProjectId('12345');
+    setTimeout(() => {


Don't set a timeout.
Change the definition of saveProjectId.

var isSaved = async saveProjectId('12345'); // async is only used for tests. In src, we don't to read back the value

Source (with async):

/** * Saves the script ID in the project dotfile. * @param {string} scriptId The script ID */ export async function saveProjectId(scriptId: string): void { return DOTFILE.PROJECT().write({ scriptId }); // Save the script id }

saveProjectId uses dotf .write(), which returns a Promise.

Not sure if that's the exact syntax, but something like that would be better than a 3000ms read file timeout.

grant · 2018-05-04T15:20:10Z

tests/test.ts

+  });
+});
+
+// NOTE: we should make saveProjectId async because dotf.write is async


Remove this note.

Make saveProjectId async function Test it (without using timeout) Signed-off-by: campionfellin <campionfellin@gmail.com>

campionfellin · 2018-05-05T01:43:29Z

All comments are covered except converting saveProjectId to saveScriptId because that needs to be changed in many places. Should be a separate PR.

grant

Stellar job!

campionfellin added 2 commits May 2, 2018 09:03

Added more tests and un-skipped ones that can run without creds.

41f9937

Also, noted that saveProjectId should be async or return a promise Signed-off-by: campionfellin <campionfellin@gmail.com>

Fix lint errors

2e68c65

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

googlebot added the cla: yes label May 2, 2018

campionfellin commented May 2, 2018

View reviewed changes

grant reviewed May 4, 2018

View reviewed changes

campionfellin self-assigned this May 5, 2018

Fix errors in PR 158

bf7e2b1

Make saveProjectId async function Test it (without using timeout) Signed-off-by: campionfellin <campionfellin@gmail.com>

grant approved these changes May 5, 2018

View reviewed changes

grant merged commit 39e0690 into google:master May 5, 2018

campionfellin deleted the more-tests branch May 25, 2018 22:55

snyk-bot mentioned this pull request Sep 13, 2021

[Snyk] Fix for 1 vulnerabilities amymariaparker2401/clasp#7

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more tests #158

Add more tests #158

campionfellin commented May 2, 2018

campionfellin May 2, 2018

grant May 4, 2018

campionfellin May 5, 2018

campionfellin May 2, 2018

grant May 4, 2018

campionfellin May 5, 2018

coveralls commented May 2, 2018 •

edited

Loading

coveralls commented May 2, 2018

coveralls commented May 2, 2018

grant left a comment

grant May 4, 2018

grant May 4, 2018

campionfellin May 5, 2018

grant May 4, 2018

grant May 4, 2018

campionfellin May 5, 2018

campionfellin commented May 5, 2018

grant left a comment

Add more tests #158

Add more tests #158

Conversation

campionfellin commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 2, 2018 • edited Loading

Pull Request Test Coverage Report for Build 146

💛 - Coveralls

coveralls commented May 2, 2018

Pull Request Test Coverage Report for Build 144

💛 - Coveralls

coveralls commented May 2, 2018

Pull Request Test Coverage Report for Build 144

💛 - Coveralls

grant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

campionfellin commented May 5, 2018

grant left a comment

Choose a reason for hiding this comment

coveralls commented May 2, 2018 •

edited

Loading