Run full suite of tests, every time (most of the time) #246

campionfellin · 2018-06-28T16:11:12Z

Fixes longstanding testing issues

See https://travis-ci.org/campionfellin/clasp/jobs/397869625 for example

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

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

campionfellin · 2018-06-28T16:13:25Z

With this, we probably would have noticed issues with Node 6/7 earlier.

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

campionfellin · 2018-06-28T16:37:32Z

Looks like manually rebuilding doesn't run them either.

Related blog post: https://blog.algolia.com/travis-encrypted-variables-external-contributions/

grant

Looks good. Thanks!

Can we add a tests/README.md:
- Explaining/providing
  - How the tests work
  - What this .clasprc.json.en is
  - How to configure Travis (if necessary)
  - Any links that are useful.
- Moving the bottom (line 327) comment to the README.

We can move testing instructions from the main README to the tests/ README too.

This will help others

grant · 2018-06-30T18:51:28Z

.clasprc.json.enc

@@ -0,0 +1,2 @@
+�t�@�NM����.|9%�	�M�mc)eJ�&���,nv�9��N����7=b����C,P8�O��A��T�,SЇ	��f�4wk�gn�<��2�p9p��ץ�p�Y�d|K�Z(�xn�T(��"��g���={)3�(�y��O���S��


Can we move this file to tests/ and update the relative path in the .travis.yaml?

This file is only used in our tests

Let's try to reduce the number of top-level files/folders.

grant · 2018-06-30T18:52:11Z

.travis.yml

 cache:
  directories:
  - "$HOME/.npm"
 before_install:
-  - npm install -g npm@latest
+- openssl aes-256-cbc -K $encrypted_0f9bbf7a60f4_key -iv $encrypted_0f9bbf7a60f4_iv


Be sure to describe exactly what this command does in the README.
It looks very complicated.

grant · 2018-06-30T18:53:04Z

.travis.yml

-  - npm run lint
-  - npm run test
+- npm run lint
+- 'if [ "$TRAVIS_PULL_REQUEST" == "false" ]; then npm run test; fi'


Can we not change this?
This only runs Travis tests for PRs and not commits?

grant · 2018-06-30T18:59:54Z

Assuming this is WIP given the do not merge label.

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

coveralls · 2018-07-01T14:43:58Z

Coverage increased (+12.6%) to 37.257% when pulling fa80380 on campionfellin:master into 1b04371 on google:master.

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

campionfellin · 2018-07-01T15:06:18Z

Ok, I think this is now working as we want:

First, see https://travis-ci.org/google/clasp/builds/398823862

If you dive into each job, it skips the tests that need authentication and ends with 11 passing and 17 pending.

Now, see https://travis-ci.org/campionfellin/clasp/builds/398823851

If you look closely, it has 27 passing, 1 pending. It also fails builds on Node 6/7 as we expect.

The logic for this is now:

const isPR = process.env.TRAVIS_PULL_REQUEST;
.
.
.
  before(function() {
    if (isPR !== 'false') {
      this.skip();
    }
  });

For some reason, $TRAVIS_PULL_REQUEST is not set to false, but rather "false" when it's not a PR.

Note, this may cause some problems with Coveralls, as the PRs will have much lower coverage, and then it will jump back up on merging with master.

grant · 2018-07-01T17:05:24Z

README.md

-sudo npm run build;
-npm run test
-```
+See `/tests/README.md`


Write a tl;dr for how to run tests.

I'd keep:

sudo npm run build; npm run test

I don't see this in the tests README.

It's under the Configuration Locally header.

grant · 2018-07-01T17:05:40Z

README.md

@@ -345,12 +345,7 @@ clasp <command>

 #### Run Tests (experimental)


Can we graduate the tests from experimental?

grant · 2018-07-01T17:08:35Z

tests/README.md

@@ -0,0 +1,63 @@
+# Tests
+


Add a sentence here to describe what the testing situation is for clasp. Something like:

Many of clasp's CLI commands have unit tests that run the clasp command and asserts the std output and std error code is as expected.

grant · 2018-07-01T17:08:53Z

tests/README.md

+# Tests
+
+These tests should ideally be run for every Pull Request, though due to the need to run `clasp login` before some commands, some tests are not run.
+


grant · 2018-07-01T17:09:24Z

tests/README.md

+
+# Configuration using Travis
+
+This is a bit more difficult, and why we include the `.clasprc.json.enc` file. The `.clasprc.json` file is used to authenticate many of the `clasp` commands, and is what is generated after using `clasp login`.


Add an intro sentence as to what Travis is and how clasp uses it.

grant · 2018-07-01T17:26:13Z

tests/README.md

+
+Change test files to use a flag that runs certain tests (what we're currently running) on PRs, and runs all tests on master.
+
+# Configuration Locally


## Local Configuration

grant · 2018-07-01T17:26:30Z

tests/README.md

+Change test files to use a flag that runs certain tests (what we're currently running) on PRs, and runs all tests on master.
+
+# Configuration Locally
+


Add a short sentence to what this section describes.

grant · 2018-07-01T17:28:09Z

tests/README.md

+# Configuration Locally
+
+1. Make sure you are logged in (`clasp login`).
+1. In `/tests/test.ts` change `describe.skip(...)` to `describe(...)` for relevant tests.


I don't see skipped test suites anymore. Can you remove this line?

grant · 2018-07-01T17:28:18Z

tests/README.md

+
+1. Make sure you are logged in (`clasp login`).
+1. In `/tests/test.ts` change `describe.skip(...)` to `describe(...)` for relevant tests.
+* **Note: (All tests are relevant).**


Remove this.

grant · 2018-07-01T17:29:54Z

tests/test.ts

@@ -28,7 +29,12 @@ describe('Test help for each function', () => {
  });
 });

-describe.skip('Test clasp list function', () => {
+describe('Test clasp list function', () => {
+  before(function() {


Can you extract this to a function?

function skipIfPR() { if (!Boolean(isPR)) return this.skip(); }

before(skipIfPR);

Not quite sure this will work, Typescript is not a fan:

[ts] 'this' implicitly has type 'any' because it does not have a type annotation.

I'm not sure the best way to pass this through that makes it work.

Also, isPR is never a boolean, it's either an int like this PR is 246 or it is "false" (string)

OK, we can follow up in a different change.

grant · 2018-07-01T17:36:31Z

Looking good.
I'd just be sure to make the instructions very short and precise, more like an instruction manual and less like a conversation.

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

campionfellin · 2018-07-01T18:15:54Z

Ok, I believe I addressed most of the comments and have made the instructions more concise.

campionfellin · 2018-07-01T19:17:43Z

@grant This build looks like it's failing, likely because the keys to decrypt are on my Travis account, rather than yours/clasp's. Can you follow these instructions:

https://docs.travis-ci.com/user/encrypting-files/#Automated-Encryption

Essentially:

clasp login
Then cp ~/.clasprc.json ./tests/.clasprc.json
travis encrypt-file ./tests/.clasprc.json --add

grant · 2018-07-02T21:01:54Z

I installed travis (after much troubleshooting) and re-encrypted the files.
@campionfellin Can you add those "essentially" steps to the tests/ README?

campionfellin · 2018-07-02T21:02:53Z

Sorry for all the troubles 😔, I'll go ahead and add those steps

grant · 2018-07-02T21:03:59Z

No worries, https://stackoverflow.com/a/34989655 was the solution.

campionfellin added 3 commits June 28, 2018 08:51

Full suite of tests

18386c5

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

Run tests if not pull request

3f48b8f

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

Fix travis script step

6eb2c80

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

campionfellin added the do not merge label Jun 28, 2018

campionfellin requested a review from grant June 28, 2018 16:11

googlebot added the cla: yes label Jun 28, 2018

Add || true so doesn't fail when unable to encrypt

635af38

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

grant suggested changes Jun 30, 2018

View reviewed changes

campionfellin added 4 commits July 1, 2018 07:00

Make testing README

56cb36a

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

Add comments to travis file

14075e1

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

Change location of .clasprc.json.enc

7bf76b5

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

Skip tests individually based on PR

f205984

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

campionfellin added 2 commits July 1, 2018 07:46

Fix falseness of env variable

3de8e76

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

Fix typos

8308842

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

campionfellin removed the do not merge label Jul 1, 2018

grant reviewed Jul 1, 2018

View reviewed changes

Fix based on notes

fa80380

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

grant approved these changes Jul 1, 2018

View reviewed changes

grant merged commit b7d9664 into google:master Jul 1, 2018

grant mentioned this pull request Jul 1, 2018

Update Travis Keys #247

Closed

		@@ -0,0 +1,2 @@
		�t�@�NM��.\|9%� �M�mc)eJ�&��,nv�9��N��7=b��C,P8�O��A��T�,SЇ ��f�4wk�gn�<��2�p9p��ץ�p�Y�d\|K�Z(�xn�T(��"��g��={)3�(�y��O��S��

		@@ -345,12 +345,7 @@ clasp <command>

		#### Run Tests (experimental)

		# Tests

		These tests should ideally be run for every Pull Request, though due to the need to run `clasp login` before some commands, some tests are not run.


		# Configuration using Travis

		This is a bit more difficult, and why we include the `.clasprc.json.enc` file. The `.clasprc.json` file is used to authenticate many of the `clasp` commands, and is what is generated after using `clasp login`.


		Change test files to use a flag that runs certain tests (what we're currently running) on PRs, and runs all tests on master.

		# Configuration Locally

Run full suite of tests, every time (most of the time) #246

Run full suite of tests, every time (most of the time) #246

Conversation

campionfellin commented Jun 28, 2018 • edited

campionfellin commented Jun 28, 2018

campionfellin commented Jun 28, 2018

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

grant commented Jun 30, 2018

coveralls commented Jul 1, 2018 • edited

campionfellin commented Jul 1, 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

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

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

grant Jul 1, 2018 • edited

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

grant commented Jul 1, 2018

campionfellin commented Jul 1, 2018

campionfellin commented Jul 1, 2018 • edited

grant commented Jul 2, 2018 • edited

campionfellin commented Jul 2, 2018

grant commented Jul 2, 2018

campionfellin commented Jun 28, 2018 •

edited

coveralls commented Jul 1, 2018 •

edited

grant Jul 1, 2018 •

edited

campionfellin commented Jul 1, 2018 •

edited

grant commented Jul 2, 2018 •

edited