exclude test build files from packages #2005

peter-hamilton · 2021-08-19T23:02:34Z

Description

Tests aren't being type checked because they are excluded tsconfig.json files. If this exclusion is removed type-checking is applied, but the resultant test build files need to be excluded from the package.

This PR

adds the !build/ts/**/*.test.* file exclusion the to all package.json files. build/* include pattern was changed to build in order for the exclusion pattern to work properly.
removes test exclusion from tsconfig.json in cases where there aren't type errors. Most packages have type errors that will be addressed in the future. To check just remove the test patterns from the exclude section of tsconfig.json.
Also, renamed test/ directories to tests/ across all packages. This improved consistency removes the need to match multiple patterns for tests.
- Changes beyond the rename are in this commit.

To test what files are included in a package, use npx npm-packlist from the package directory.

Type of change

in all Patch: Bug (non-breaking change which fixes an issue)

Checklist

I have added a changelog entry, prefixed by the type of change noted above (Documentation fix and Test update does not need a changelog as we do not publish new version)

dahukish · 2021-08-23T16:50:09Z

packages/address-mocks/package.json

    "!build/*.tsbuildinfo",
+    "!build/ts/**/*.test.*",
    "index.js",


I think index.js get added but default no?

https://docs.npmjs.com/cli/v7/configuring-npm/package-json#files

Unless we are being explicit for clarity.

That's true. Though I'd lean towards keeping it explicit.

dahukish · 2021-08-23T17:41:13Z

packages/graphql-tool-utilities/tsconfig.json

-    "./src/**/*.tsx"
-  ],
-  "exclude": ["**/*.test.ts", "**/*.test.tsx"]
+  "include": ["../../config/typescript/*.ts", "./src/**/*.ts", "./src/**/*.tsx"]


could these go in the base config if most packages follow the same pattern?

dahukish

LGTM

BPScott · 2021-08-23T18:15:09Z

!build/ts/**/*.test.* is good for excluding test files. I wonder if we should exclude the whole test / tests folders, as sometimes those folders contain utility files that are only used within tests: ls -d packages/*/src/**/test{,s}/** | grep -v '\.test\.' give you a list of them. Do you think doing "!build/ts/**/tests/" instead would be worth it?

Annoyingly it seems half of our tests live in folders called test and half live folders called tests (ls -d packages/*/src/**/tests | wc -l vs ls -d packages/*/src/**/test | wc -l). the Shopify convention tends to be naming tests folders tests (3999 usages of tests in web, vs 6 for test). That's pretty annoying as ignoring both folder names in package.json feels rubbish. Renaming all instances for test folders to tests is also gonna be noisy but I'm a sucker for conventions. Reckon it might be worth it?

BPScott · 2021-08-23T22:51:54Z

.eslintignore

-packages/graphql-config-utilities/src/tests/fixtures/**/*.graphql
-packages/graphql-typescript-definitions/test/fixtures/**/*.graphql
-packages/graphql-validate-fixtures/test/fixtures/**/*.graphql
+packages/graphql-config-utilities/src/testss/fixtures/**/*.graphql


Double s here might be the cause of the linting failures

Thank you. This was driving me nuts.

peter-hamilton · 2021-08-24T15:10:23Z

!build/ts/**/*.test.* is good for excluding test files. I wonder if we should exclude the whole test / tests folders, as sometimes those folders contain utility files that are only used within tests: ls -d packages/*/src/**/test{,s}/** | grep -v '\.test\.' give you a list of them. Do you think doing "!build/ts/**/tests/" instead would be worth it?

Annoyingly it seems half of our tests live in folders called test and half live folders called tests (ls -d packages/*/src/**/tests | wc -l vs ls -d packages/*/src/**/test | wc -l). the Shopify convention tends to be naming tests folders tests (3999 usages of tests in web, vs 6 for test). That's pretty annoying as ignoring both folder names in package.json feels rubbish. Renaming all instances for test folders to tests is also gonna be noisy but I'm a sucker for conventions. Reckon it might be worth it?

test folders have been renamed to tests.

BPScott

Looking pretty great!

I think I spotted two little things to tweak inline. fixup those two and I think we're good to go!

I see there's till many mentions of **/test/** in tsconfig files but I guess those are the problematic packages and those entire exclude declarations shall be removed in follow up PRs

packages/csrf-token-fetcher/tsconfig.json

BPScott · 2021-08-24T15:58:23Z

packages/storybook-a11y-test/tsconfig.json

@@ -4,6 +4,5 @@
    "outDir": "build/ts",
    "rootDir": "src",
    "baseUrl": "src"
-  },
-  "include": ["./src/**/*.ts"]


i think this needs to stay

Co-authored-by: Ben Scott <227292+BPScott@users.noreply.github.com>

peter-hamilton added 3 commits August 19, 2021 18:45

exclude test build files from packages

f5baa07

apply type-check to tests without type errors

5487ad1

update change logs

f7d8e26

peter-hamilton requested a review from a team August 19, 2021 23:54

peter-hamilton marked this pull request as ready for review August 19, 2021 23:57

change 'build' to 'build/' file pattern for clarity

bb2a145

dahukish reviewed Aug 23, 2021

View reviewed changes

dahukish approved these changes Aug 23, 2021

View reviewed changes

peter-hamilton added 2 commits August 23, 2021 17:09

rename test/ to tests/

9d6356c

fix test/ references

b99480e

BPScott reviewed Aug 23, 2021

View reviewed changes

peter-hamilton added 2 commits August 23, 2021 19:19

fix typo

ff8391b

update test exclude for packages

ea41516

BPScott reviewed Aug 24, 2021

View reviewed changes

peter-hamilton and others added 2 commits August 24, 2021 12:09

Update packages/csrf-token-fetcher/tsconfig.json

ec92fa2

Co-authored-by: Ben Scott <227292+BPScott@users.noreply.github.com>

revert accidental changes to storybook-a11y-test

eb05a85

BPScott approved these changes Aug 24, 2021

View reviewed changes

peter-hamilton merged commit 72e53fe into main Aug 24, 2021

peter-hamilton deleted the exclude-test-files branch August 24, 2021 16:21

shopify-shipit bot temporarily deployed to production August 24, 2021 19:46 Inactive

shopify-shipit bot temporarily deployed to michenly-beta August 26, 2021 17:50 Inactive

shopify-shipit bot temporarily deployed to gem August 30, 2021 18:42 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exclude test build files from packages #2005

exclude test build files from packages #2005

peter-hamilton commented Aug 19, 2021 •

edited

Loading

dahukish Aug 23, 2021 •

edited

Loading

dahukish Aug 23, 2021

peter-hamilton Aug 23, 2021

dahukish Aug 23, 2021

dahukish left a comment

BPScott commented Aug 23, 2021

BPScott Aug 23, 2021

peter-hamilton Aug 23, 2021

peter-hamilton commented Aug 24, 2021

BPScott left a comment •

edited

Loading

BPScott Aug 24, 2021

peter-hamilton Aug 24, 2021

exclude test build files from packages #2005

exclude test build files from packages #2005

Conversation

peter-hamilton commented Aug 19, 2021 • edited Loading

Description

Type of change

Checklist

dahukish Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

dahukish Aug 23, 2021

Choose a reason for hiding this comment

peter-hamilton Aug 23, 2021

Choose a reason for hiding this comment

dahukish Aug 23, 2021

Choose a reason for hiding this comment

dahukish left a comment

Choose a reason for hiding this comment

BPScott commented Aug 23, 2021

BPScott Aug 23, 2021

Choose a reason for hiding this comment

peter-hamilton Aug 23, 2021

Choose a reason for hiding this comment

peter-hamilton commented Aug 24, 2021

BPScott left a comment • edited Loading

Choose a reason for hiding this comment

BPScott Aug 24, 2021

Choose a reason for hiding this comment

peter-hamilton Aug 24, 2021

Choose a reason for hiding this comment

peter-hamilton commented Aug 19, 2021 •

edited

Loading

dahukish Aug 23, 2021 •

edited

Loading

BPScott left a comment •

edited

Loading