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 clasp status tests and --json option #167

Merged
merged 4 commits into from May 15, 2018

Conversation

ajlai
Copy link
Contributor

@ajlai ajlai commented May 15, 2018

Adds a working test case for clasp status, a failing test case for #67 and implements #163

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

@coveralls
Copy link

coveralls commented May 15, 2018

Pull Request Test Coverage Report for Build 160

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 265 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.006%) to 14.082%

Files with Coverage Reduction New Missed Lines %
tests/test.js 49 31.58%
index.js 216 6.56%
Totals Coverage Status
Change from base Build 154: -0.006%
Covered Lines: 163
Relevant Lines: 887

💛 - Coveralls

@grant grant requested a review from campionfellin May 15, 2018 05:54
index.ts Outdated
console.log(`└─ ${filePath}`);
});
if (ignoredFilePaths.length) {
const status = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep this format.
const [filesToPush, untrackedFiles] = projectFiles;

Below use:

if (cmd.json) {
  console.log(JSON.stringify({ filesToPush, untrackedFiles});

@grant
Copy link
Contributor

grant commented May 15, 2018

Thanks @ajlai. LGTM in general.
@campionfellin Do you want to look at this PR? Feel free to hand off to me if you don't want to.

Copy link
Collaborator

@campionfellin campionfellin left a comment

Choose a reason for hiding this comment

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

LGTM, pending a couple of comments I made.

@@ -106,6 +107,47 @@ describe.skip('Test clasp push function', () => {
});
});

describe.skip('Test clasp status function', () => {
function setupTmpDirectory(filepathsAndContents: Array<{ file: string, data: string }>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious why we need a /tmp directory for testing. We've been using fs.writeFileSync('.clasprc.json', ' '); if we need files for testing. Should we be changing that in the other tests as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the other tests using fs.writeFileSync directly actually just end up dumping files into the /clasp directory. It'd probably be better to have all the test related file writing in tmp/ so we can clean up nicely afterwards.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

expect(resultJson.untrackedFiles).to.have.members(['shouldBeIgnored', 'should/alsoBeIgnored']);
expect(resultJson.filesToPush).to.have.members(['build/main.js', 'appsscript.json']);
});
// https://github.com/google/clasp/issues/67 - This test currently fails
Copy link
Collaborator

Choose a reason for hiding this comment

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

After each test, maybe add fs.removeSync('/tmp'); to clean up a bit?

Copy link
Contributor Author

@ajlai ajlai May 15, 2018

Choose a reason for hiding this comment

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

The keep: false here cleans up tmpdir after the tests finish: https://github.com/google/clasp/pull/167/files#diff-9c706fb89ddc3725ae3168d13b1298eaR113

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perfect, thanks for changing that.

@campionfellin
Copy link
Collaborator

LGTM, thanks @ajlai, great work on this PR!

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

5 participants