Initial test structure #7

aliok · 2018-12-05T11:32:16Z

https://issues.jboss.org/browse/AEROGEAR-8183

Changes:

Introduced ava in all packages and setup their test scripts and ava settings. For the packages that don't have any tests yet, I created a "deleteme.test.js" file so that ava doesn't complain
Moved 2 tests:
- packages/apollo-voyager-tools/src/buildPath.test.ts
- packages/apollo-voyager-keycloak/src/schemaDirectives/hasRole.test.ts
Made lint command don't check tests
But, made format command to also format tests (--> we could actually create a Git commit hook for this)

As per https://github.com/lerna/lerna#common-devdependencies

Note that devDependencies providing "binary" executables that are used by npm scripts still need to be installed directly in each package where they're used.

we need to have ava and typescript as devDeps in subpackages.

CircleCI is happy.

aliok · 2018-12-05T11:34:51Z

@darahayes @wtrocki let's merge this one and then I will do these separately:

~~try setting up on-the-fly compilation for tests with Ava~~
move some more tests

Update: I was thinking we can have both on-the-fly compilation and the regular approach but it doesn't work that way. I changed the approach and decided to use on the fly compilation already in this initial PR

wtrocki · 2018-12-05T12:14:13Z

@aliok This looks good, but there is chance that we missing git ignore for package lock files.
This will need to be rebased after @darahayes fix will be merged.
Apart from that change looks great. I would personally add test command into each package instead of exit 0 but that is optional (it will not fail even when there are no tests so next PR's will focus on adding tests to those packages.

wtrocki · 2018-12-05T12:16:09Z

Out of curiosity: Do we want to lint unit tests as well? I believe that this will cause so much hassle that it is not worth it. We can disable lint in separate PR (package.json filter), but wanted to trigger discussion

aliok · 2018-12-05T12:56:28Z

@wtrocki

Apart from that change looks great. I would personally add test command into each package instead of exit 0 but that is optional (it will not fail even when there are no tests so next PR's will focus on adding tests to those packages.

Let me do that separately, if @darahayes and you are ok with the structure.

Out of curiosity: Do we want to lint unit tests as well? I believe that this will cause so much hassle that it is not worth it. We can disable lint in separate PR (package.json filter), but wanted to trigger discussion

Honestly, part of me says they're code so we should lint them. But on the other hand I just want to have the ability to pass mock/inpartial objects as 'any' without complaints.
Let me disable linting for tests.

wtrocki · 2018-12-05T13:19:06Z

I have done that in #9

aliok · 2018-12-05T13:33:34Z

Apart from that change looks great. I would personally add test command into each package instead of exit 0 but that is optional (it will not fail even when there are no tests so next PR's will focus on adding tests to those packages.

Nope, this is not the case:

✖ Couldn't find any files to test

I will add dummy files

aliok · 2018-12-05T13:45:02Z

@darahayes @wtrocki this PR is ready for the final review.
#9 is included here but with slight improvement.

darahayes

@aliok changes look great! I was just wondering with respect to the linting of tests. Perhaps we could just create a separate lint config for tests that is more lenient on things like any but it still checks for stylistic + formatting issues. What do you guys think?

darahayes · 2018-12-05T14:42:03Z

package.json

-    "lint": "tslint '*/*/src/**/*.ts' '*/*/test/**/*.ts'",
-    "format": "tslint '*/*/src/**/*.ts' '*/*/test/**/*.ts' --fix",
+    "lint": "tslint '*/*/src/**/*.ts' --exclude '*/*/src/**/*.test.ts'",
+    "format": "tslint '*/*/src/**/*.ts' --fix --force > /dev/null",


Just wondering what's the > /dev/null thing for here? Is this going to suppress error output?

Short answer yes:

Long answer

2 scripts:

lint: Complains with "ERROR: bla bla" messages, doesn't fix, exits with non-zero in case of error, only used in non-test code

format: It runs on test and non-test code. Don't want to see "ERROR: bla bla" in the output because the tests might have too many of them, so that's why I put "> dev/null". I don't want it to return non-zero, so I put "--force".

darahayes · 2018-12-05T14:42:39Z

packages/apollo-voyager-context/src/deleteme.test.ts

@@ -0,0 +1,5 @@
+import test from 'ava'
+
+test('DELETE ME DUMMY', t => {


darahayes · 2018-12-05T14:43:51Z

packages/apollo-voyager-keycloak/src/schemaDirectives/hasRole.test.ts

@@ -0,0 +1,249 @@
+import test from 'ava'
+
+import {GraphQLSchema} from 'graphql'


Really minor thing but just check spacing around imports like this. I'm wondering is this something your IDE does? I wonder can we lint for this.

Yeah, my IDE does format like that.
But I was assuming it is using eslint. Let me have a look at this ...tomorrow.

@darahayes I tried tslint's --fix now and it doesn't fix the spaces for that case.
I tried bunch of tslint rules to fix/enforce that behavior but I wasn't successful.

UPDATE: I found this open issue on tslint: palantir/tslint#1044

Anyway, I will fix this manually.

aliok · 2018-12-05T14:58:14Z

@darahayes

Perhaps we could just create a separate lint config for tests that is more lenient on things like any but it still checks for stylistic + formatting issues. What do you guys think?

Sounds good. I think we can create a separate rule set for the tests.
... or, we can use the format script I provided with a Git hook so that it doesn't break things in case of any but it can fix style&format.

darahayes · 2018-12-05T17:29:41Z

@aliok I'm personally -1 on trying to do formatting as part of a commit hook because the formatting will be run after you've already git added your changes and I don't think we should try to auto git add stuff because that would likely lead to adding the wrong things.

I recommend we add a separate lint config for tests. Eventually we will add a commit hook that only does linting but no formatting. What do you think? :)

aliok · 2018-12-06T07:02:42Z

@aliok I'm personally -1 on trying to do formatting as part of a commit hook because the formatting will be run after you've already git added your changes and I don't think we should try to auto git add stuff because that would likely lead to adding the wrong things.

OK, I don't have a strong opinion in terms of Git hook.

I recommend we add a separate lint config for tests. Eventually we will add a commit hook that only does linting but no formatting. What do you think? :)

I can try crteating a separate lint config, but in another PR. Let's merge this one first.

@darahayes give me your approval!

darahayes

😄

aliok requested review from darahayes and wtrocki December 5, 2018 11:33

aliok force-pushed the AEROGEAR-8183-test-setup branch 2 times, most recently from 562a76a to b73e168 Compare December 5, 2018 11:54

AEROGEAR-8183 initial test structure

f62b8ca

aliok force-pushed the AEROGEAR-8183-test-setup branch from b73e168 to f62b8ca Compare December 5, 2018 12:54

aliok mentioned this pull request Dec 5, 2018

fix: Disable lint for test that can lead to unnecessary improvements #9

Closed

AEROGEAR-8183 initial test structure

bfbad24

aliok force-pushed the AEROGEAR-8183-test-setup branch from eb55dc2 to bfbad24 Compare December 5, 2018 13:47

darahayes reviewed Dec 5, 2018

View reviewed changes

AEROGEAR-8183 initial test structure

e70a615

aliok mentioned this pull request Dec 6, 2018

Aerogear 8183 test setup 2 #10

Merged

darahayes approved these changes Dec 6, 2018

View reviewed changes

aliok merged commit 1389e55 into master Dec 6, 2018

aliok deleted the AEROGEAR-8183-test-setup branch December 6, 2018 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial test structure #7

Initial test structure #7

aliok commented Dec 5, 2018 •

edited

Loading

aliok commented Dec 5, 2018 •

edited

Loading

wtrocki commented Dec 5, 2018

wtrocki commented Dec 5, 2018

aliok commented Dec 5, 2018

wtrocki commented Dec 5, 2018

aliok commented Dec 5, 2018

aliok commented Dec 5, 2018

darahayes left a comment

darahayes Dec 5, 2018

aliok Dec 5, 2018

darahayes Dec 5, 2018

darahayes Dec 5, 2018

aliok Dec 5, 2018

aliok Dec 6, 2018

aliok commented Dec 5, 2018

darahayes commented Dec 5, 2018

aliok commented Dec 6, 2018

darahayes left a comment

		@@ -0,0 +1,5 @@
		import test from 'ava'

		test('DELETE ME DUMMY', t => {

		@@ -0,0 +1,249 @@
		import test from 'ava'

		import {GraphQLSchema} from 'graphql'

Initial test structure #7

Initial test structure #7

Conversation

aliok commented Dec 5, 2018 • edited Loading

aliok commented Dec 5, 2018 • edited Loading

wtrocki commented Dec 5, 2018

wtrocki commented Dec 5, 2018

aliok commented Dec 5, 2018

wtrocki commented Dec 5, 2018

aliok commented Dec 5, 2018

aliok commented Dec 5, 2018

darahayes left a comment

Choose a reason for hiding this comment

darahayes Dec 5, 2018

Choose a reason for hiding this comment

aliok Dec 5, 2018

Choose a reason for hiding this comment

Long answer

darahayes Dec 5, 2018

Choose a reason for hiding this comment

darahayes Dec 5, 2018

Choose a reason for hiding this comment

aliok Dec 5, 2018

Choose a reason for hiding this comment

aliok Dec 6, 2018

Choose a reason for hiding this comment

aliok commented Dec 5, 2018

darahayes commented Dec 5, 2018

aliok commented Dec 6, 2018

darahayes left a comment

Choose a reason for hiding this comment

aliok commented Dec 5, 2018 •

edited

Loading

aliok commented Dec 5, 2018 •

edited

Loading