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

Prep testlab for publishing #219

Merged
merged 1 commit into from
Apr 26, 2017
Merged

Conversation

superkhau
Copy link
Contributor

@superkhau superkhau commented Apr 25, 2017

  • Add unit test to ensure exported module works as expected

cc @bajtos @raymondfeng

@superkhau superkhau requested a review from ritch April 25, 2017 23:55
@superkhau superkhau added this to the Sprint 34 - Asteroid milestone Apr 25, 2017
@superkhau superkhau changed the title Add acceptance test for @loopback/loopback export Prep testlab for publishing Apr 25, 2017
@superkhau superkhau force-pushed the prep-testlab-for-publishing branch 3 times, most recently from 3ade9b5 to 557063e Compare April 26, 2017 00:00
```js
const Application = require('@loopback/loopback').Application;
const app = new Application();
```
Copy link
Contributor Author

@superkhau superkhau Apr 26, 2017

Choose a reason for hiding this comment

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

Ignore this, I'm branching off another branch that has these files. Will disappear once I land #213 and rebase.

server.bind('applications.myApp').to(app);
return new Client(server);
}
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These too -- they will be gone after rebase.

@superkhau superkhau force-pushed the prep-testlab-for-publishing branch 4 times, most recently from 92801b9 to 9621bf1 Compare April 26, 2017 00:11
@superkhau superkhau self-assigned this Apr 26, 2017
const should = require('should');
const testlab = require('..');

describe('testlab', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should not here that this is a JS smoke test - not a real test

Copy link
Contributor Author

@superkhau superkhau Apr 26, 2017

Choose a reason for hiding this comment

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

Well it's using the real compiled files, but yes a true test would have to require the dep from NPMJS (ie. add testlab as a true dep via npm install -S testlab). Renaming to smoke test for clarity.

"clean": "rm *.tgz; rm -rf ./package",
"prepublish": "npm run build",
"test": "mocha",
"verify": "npm run clean && npm pack && tar xvzf *.tgz && tree package"
Copy link
Contributor

@ritch ritch Apr 26, 2017

Choose a reason for hiding this comment

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

is this for local testing only? tree is only avail. via brew

Copy link
Contributor Author

@superkhau superkhau Apr 26, 2017

Choose a reason for hiding this comment

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

Yes, only for local testing to make sure the packaged contents are what we expect. tar is not available on Windows too, but this NPM script should be temporary. Will remove once we get more insight into final publishing routines.

@superkhau superkhau force-pushed the prep-testlab-for-publishing branch 2 times, most recently from 6eb3da1 to 5567a5f Compare April 26, 2017 00:26
- Add smoke test to ensure exported module works as expected
@ritch ritch mentioned this pull request Apr 26, 2017
3 tasks
@superkhau superkhau merged commit 5790498 into master Apr 26, 2017
@superkhau superkhau deleted the prep-testlab-for-publishing branch April 26, 2017 00:45
@superkhau superkhau removed the review label Apr 26, 2017
package
src
test
tsconfig.json
Copy link
Member

Choose a reason for hiding this comment

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

I think we should be using an explicit white-list of files to include in the package via package.json files property, rather than maintaining a backlist in .npmignore.

Copy link
Member

Choose a reason for hiding this comment

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

Note that the list of files can specify directories too, e.g.

"files": [
  "index.js",
  "index.d.ts",
  "lib"
]

"module": "commonjs",
"moduleResolution": "node",
"outDir": "./lib",
"strictNullChecks": true,
Copy link
Member

Choose a reason for hiding this comment

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

Please don't duplicate this configuration, inherit it from top-level tsconfig.json instead.

Ideally, the project-level tsconfig.json should configure only include and exclude, all compilerOptions should be inherited.

Copy link
Member

Choose a reason for hiding this comment

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

done - see #220

@bajtos
Copy link
Member

bajtos commented Apr 26, 2017

@superkhau I am afraid this build the type information when running npm test at the root level. When I enable noImplicitAny, npm test is failing with the following error:

packages/authentication/test/acceptance/authentication/routing.acceptance.ts (8,22):
Could not find a declaration file for module 'testlab'.'
'/Users/bajtos/src/loopback/next/packages/testlab/index.js' implicitly has an 'any' type. (7016)

I.e. when you import * from @loopback/testlab, you will loose type information. I'll fix this problem myself, I am leaving the comment here just to make us aware of this issue.

@bajtos
Copy link
Member

bajtos commented Apr 26, 2017

I'll fix this problem myself, I am leaving the comment here just to make us aware of this issue.

See d41ee54

@emonddr emonddr mentioned this pull request Nov 1, 2019
24 tasks
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

4 participants