build(testlab): move test files to `src/tests` #2283

bajtos · 2019-01-24T12:30:44Z

I'd like to resume work on migrating our monorepo to TypeScript Project References, as investigated in #1636. As I see it, the next step is to simplify our project setup by moving our test files from test to src/__test__.

Source files are compiled from src/{foo} to dist/{foo}, the same pattern is applied to test files too.
Both TypeScript sources and JavaScript output are stored in the same path relative to project root. This makes it much easier to refer to test fixtures.
~~There is no need for hacky manually-mainitained index files in package root. We can directly use TypeScript output: dist/index.js as the main package file, dist/index.d.ts as the type definitions.~~

This change is also enabling future improvements, for example TypeScript project references and migration to jest test runner.

IMPORTANT NOTE

I am intentionally migrating only one of our packages in this pull request. By limiting the changes to a single package, the pull request is smaller and easier to review in detail. Once we agree on the best way how to make these changes, I'll open follow-up pull requests to migrate other monorepo packages accordingly.

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

lerna.json

bajtos · 2019-01-24T12:32:21Z

packages/testlab/package.json

    "should-as-function.d.ts",
-    "src"
+    "src",
+    "!*/__tests__"


This excludes both src/__tests__ and dist/__tests__ from the npm package tarball.

bajtos · 2019-01-24T15:05:29Z

I just realized that I forgot to check that developer experience in VS Code is preserved with these changes in place, per https://github.com/strongloop/loopback-next/blob/master/docs/site/DEVELOPING.md#how-to-test-infrastructure-changes

This PR should not be landed until I verify those aspects.

bajtos · 2019-01-25T07:34:40Z

As kind of expected, "Go to definition" in VSCode is jumping to .d.ts files after this change :(

bajtos · 2019-01-25T07:47:58Z

"Go to definition" in VSCode is jumping to .d.ts files

This can be easily fixed by adding "declarationMap": true to compiler options in packages/build/config/tsconfig.common.json.

Unfortunately, that does not solve the second important feature: refactorings, e.g. "rename symbol".

bajtos · 2019-01-25T07:56:18Z

To make refactorings work, I had to bring package-level index files back. I hope we will be able to remove them when we switch to project references and abandon the current approach where we pretend for VSCode that the entire monorepo is a single typescript project.

Original commit: c14e4ea
Changes to make refactoring work: ee10452

bajtos · 2019-01-25T08:01:58Z

@raymondfeng the patch is ready for final review and landing, PTAL.

raymondfeng · 2019-01-25T17:13:16Z

@bajtos The following settings in package.json seem to be working for me if we remove index.* at root level.

"main": "./dist/index.js",
"types": "./src/index.ts",

Can you verify?

raymondfeng · 2019-01-25T17:14:27Z

What's going to happen for fixtures? I'm seeing two fixtures under testlab:

/fixtures
/src/tests/fixtures

bajtos · 2019-01-28T09:51:29Z

@raymondfeng

The following settings in package.json seem to be working for me if we remove index.* at root level.
"main": "./dist/index.js",
"types": "./src/index.ts",
Can you verify?

Nice hack! It works for me well 👍

I am concerned about the ramifications of such change for our consumers though, I posted a question on StackOverflow to get a qualified answer (hopefully ): https://stackoverflow.com/questions/54399058/ramifications-of-exporting-original-ts-files-instead-of-d-ts-files-as-module-t. Feel free to post an answer yourself.

Considering that switching from hand-written /index* files to "types": "./src/index.ts" is a very small change that should not introduce any merge conflicts, I prefer to leave it out of scope of this pull request and wait until we have better understanding of the problem.

I think that once we switch to project references, we may not need this hack anymore 🤞 , so it may be better to not bother at all and remove /index* files as part of switching to project references.

Thoughts?

What's going to happen for fixtures? I'm seeing two fixtures under testlab:

/fixtures

/src/tests/fixtures

The directory /fixtures contains files that we don't want TypeScript to compile and files that TypeScript cannot compile (e.g. *.json, *.txt, etc.).

The directory /src/__tests__/fixtures contains TypeScript sources we want to compile to JavaScript.

What do you think, how can we make this distinction easier to understand for people contributing to LoopBack (including ourselves)? Few alternatives that come to my mind:

Create two README files, one in each fixtures directory, and explain what kind of files belongs to each directory.
Add an entry to README's section Testing.
Add a new section to DEVELOPING.md
Any other ideas?

raymondfeng · 2019-01-28T22:05:38Z

Before this PR, we have root level index.ts (export * from './src';) and index.d.ts (export * from './dist';). Without running npm build, ./dist does not exist. If VSCode is happy with our project layout, I think it gets types from index.ts.

Please also note that sourcemap files do not contain TypeScript source code as far as I see from ./dist/**/*.map.

raymondfeng · 2019-01-28T22:13:14Z

For two types of fixtures, maybe README for each directory will help.

bajtos · 2019-01-29T09:30:30Z

For two types of fixtures, maybe README for each directory will help.

Done in 232cde1.

Before this PR, we have root level index.ts (export * from './src';) and index.d.ts (export * from './dist';). Without running npm build, ./dist does not exist. If VSCode is happy with our project layout, I think it gets types from index.ts.

Here is my understanding of how thing works currently:

When working in our monorepo:

Inside VSCode, TypeScript Language Service sees packages/testlab/index.ts and uses that file for type information and navigation around our code base.
At runtime, Node.js is using packages/testlab/index.js as the entry point.

When consuming testlab as a dependency of 3rd-party LB project:

The npm package includes index.js and index.d.ts., there is no index.ts shipped to npm!
Inside VSCode, TypeScript Language Service uses node_modules/testlab/index.d.ts for type information and "go to definition".
At runtime, Node.js is using node_modules/testlab/index.js as the entry point.

Anyhow, as I commented before, I would like to set this discussion aside for now and land only the changes moving test files from test to src/__tests__.

@raymondfeng Does the pull request LGTY now?

b-admike

As far as moving test files to src/__tests__ and the rationale behind it, LGTM.

packages/testlab/fixtures/README.md

This change greatly simplifies our build and project setup. - Source files are compiled from `src/{foo}` to `dist/{foo}`, the same pattern is applied to test files too. - Both TypeScript sources and JavaScript output are stored in the same path relative to project root. This makes it much easier to refer to test fixtures. This change is also enabling future improvements, for example TypeScript project references and migration to jest test runner.

bajtos self-assigned this Jan 24, 2019

bajtos requested review from raymondfeng and a team January 24, 2019 12:30

bajtos commented Jan 24, 2019

View reviewed changes

bajtos added refactor Internal Tooling Issues related to our tooling and monorepo infrastructore labels Jan 24, 2019

bajtos changed the title ~~build(testlab): move test files to src/__tests__~~ build(testlab): move test files to src/__tests__ [WIP - DO NOT MERGE] Jan 24, 2019

bajtos force-pushed the refactor/test-directory branch from ee10452 to e68d6bf Compare January 25, 2019 08:01

bajtos changed the title ~~build(testlab): move test files to src/__tests__ [WIP - DO NOT MERGE]~~ build(testlab): move test files to src/__tests__ Jan 25, 2019

bajtos force-pushed the refactor/test-directory branch from e68d6bf to c1b1315 Compare January 25, 2019 15:28

bajtos force-pushed the refactor/test-directory branch from c1b1315 to 232cde1 Compare January 29, 2019 09:25

b-admike approved these changes Jan 29, 2019

View reviewed changes

packages/testlab/fixtures/README.md Outdated Show resolved Hide resolved

bajtos force-pushed the refactor/test-directory branch from 232cde1 to 65f247f Compare January 29, 2019 13:04

raymondfeng approved these changes Jan 29, 2019

View reviewed changes

bajtos added 3 commits January 31, 2019 08:53

docs: fix instructions for verifying TypeScript setup

4843df7

docs(testlab): describe test fixture directories

ac24e03

bajtos force-pushed the refactor/test-directory branch from 65f247f to ac24e03 Compare January 31, 2019 07:54

bajtos merged commit f946d48 into master Jan 31, 2019

bajtos deleted the refactor/test-directory branch January 31, 2019 08:11

This was referenced Jan 31, 2019

build: move test files to src/__tests__ in example apps #2316

Merged

build: move test files to src/__tests__ in packages #2330

Merged

Scaffold test files to src/__tests__ + small fixes #2339

Merged

bajtos mentioned this pull request Mar 18, 2019

Enable TypeScript Project References #2609

Closed

7 tasks

jannyHou mentioned this pull request Mar 28, 2019

Move test files to src/__tests__ loopbackio/loopback4-example-shopping#68

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build(testlab): move test files to `src/tests` #2283

build(testlab): move test files to `src/tests` #2283

bajtos commented Jan 24, 2019 •

edited

Loading

bajtos Jan 24, 2019

bajtos commented Jan 24, 2019

bajtos commented Jan 25, 2019

bajtos commented Jan 25, 2019

bajtos commented Jan 25, 2019 •

edited

Loading

bajtos commented Jan 25, 2019

raymondfeng commented Jan 25, 2019

raymondfeng commented Jan 25, 2019

bajtos commented Jan 28, 2019

raymondfeng commented Jan 28, 2019 •

edited by bajtos

Loading

raymondfeng commented Jan 28, 2019

bajtos commented Jan 29, 2019

b-admike left a comment

build(testlab): move test files to src/__tests__ #2283

build(testlab): move test files to src/__tests__ #2283

Conversation

bajtos commented Jan 24, 2019 • edited Loading

Checklist

bajtos Jan 24, 2019

Choose a reason for hiding this comment

bajtos commented Jan 24, 2019

bajtos commented Jan 25, 2019

bajtos commented Jan 25, 2019

bajtos commented Jan 25, 2019 • edited Loading

bajtos commented Jan 25, 2019

raymondfeng commented Jan 25, 2019

raymondfeng commented Jan 25, 2019

bajtos commented Jan 28, 2019

raymondfeng commented Jan 28, 2019 • edited by bajtos Loading

raymondfeng commented Jan 28, 2019

bajtos commented Jan 29, 2019

b-admike left a comment

Choose a reason for hiding this comment

build(testlab): move test files to `src/tests` #2283

build(testlab): move test files to `src/tests` #2283

bajtos commented Jan 24, 2019 •

edited

Loading

bajtos commented Jan 25, 2019 •

edited

Loading

raymondfeng commented Jan 28, 2019 •

edited by bajtos

Loading