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.

Sign up for GitHub

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

Jump to bottom

Saner test setup #27

Merged
merged 1 commit into from
Feb 2, 2017
Merged

Saner test setup #27

merged 1 commit into from
Feb 2, 2017

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Feb 2, 2017 •
edited
Loading

  • Upgrade lerna to the currently latest beta version (2.0.0-beta.35)
  • Get rid of global expect/sinon
  • Create a new package @loopback/testlab for shared test helpers
  • Configure top-level "npm test"

@superkhau @ritch please review

@bajtos bajtos self-assigned this Feb 2, 2017
@bajtos bajtos requested review from ritch and superkhau February 2, 2017 12:11
@bajtos bajtos mentioned this pull request Feb 2, 2017
Merged
@bajtos
Copy link
Member Author

bajtos commented Feb 2, 2017

See also #20 (comment)

Globals are pain to use, didn't we learn that lesson in LoopBack 2.x/3.x yet? See strongloop/loopback#2632.

@bajtos bajtos force-pushed the fix/shared-test-setup branch from 93bd5d8 to 448174e Compare February 2, 2017 12:53
@bajtos
Saner test setup
0332f66 
 - Upgrade lerna to the currently latest beta version (2.0.0-beta.35)
 - Get rid of global expect/sinon
 - Create a new package "@loopback/testlab" for shared test helpers
 - Configure top-level "npm test"
@bajtos bajtos force-pushed the fix/shared-test-setup branch from 448174e to 0332f66 Compare February 2, 2017 12:56
@bajtos
Copy link
Member Author

bajtos commented Feb 2, 2017

This pull request also fixes TypeScript config and type definitions for chai and sinon. Unfortunately, there are no type definitions for dirty-chai yet - see prodatakey/dirty-chai#21.

As a result, the following code is highlighted in VS Code:

screen shot 2017-02-02 at 13 58 44

I think we will need to contribute typings for dirty-chai if we decide to continue using TypeScript.

@ritch
Copy link
Contributor

ritch commented Feb 2, 2017

LGTM

@ritch ritch merged commit 4dc0e95 into master Feb 2, 2017
@bajtos bajtos deleted the fix/shared-test-setup branch February 2, 2017 15:57
@superkhau
Copy link
Contributor

LGTM, except one minor comment.

superkhau
superkhau reviewed Feb 2, 2017
View reviewed changes
packages/testlab/tsconfig.json
{
"extends": "../tsconfig.json",
"baseUrl": "."
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file required?

Copy link
Contributor

Choose a reason for hiding this comment

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

I removed it in a subsequent PR as everything works without this file anyways -- #30

Copy link
Member Author

Choose a reason for hiding this comment

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

At some point, TS was not detecting @types installed in test-lab's node_modules. If everything works for you without this file, then I am fine with removing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be working for me (I'm getting autocompletion for both sinon/chai with it removed). Will add back later if we run into issues.

@superkhau
Copy link
Contributor

I think we will need to contribute typings for dirty-chai if we decide to continue using TypeScript.

I'm thinking we should remove dirty-chai until:

  1. We contribute TS defs ourselves
  2. We spend time to find some other way around it

I'm also up for creating an issue and maybe getting someone in community to look into it and label it beginner friendly. Not liking the red underline on ALL expects in vscode, it's distracting and bad UX ATM.

@bajtos
Copy link
Member Author

bajtos commented Feb 3, 2017

I'm thinking we should remove dirty-chai

👎

https://github.com/moll/js-must#beware-of-libraries-that-assert-on-property-access

This is a real concern, we had real tests in loopback (juggler IIRC) that were calling undefined properties like to.be.an.object.

Not liking the red underline on ALL expects in vscode, it's distracting and bad UX ATM.

Agreed. I'll take a look at typings ASAP.

@superkhau
Copy link
Contributor

superkhau commented Feb 3, 2017 •
edited
Loading

Agreed. I'll take a look at typings ASAP.

I agree with why we added dirty-chai. Should've been more specific and said remove it until we add the TS defs (reintroduce it then). You fixed it all last night anyways, so it's all good now. 👍 FYI, it all works great locally for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@superkhau superkhau superkhau left review comments

@ritch ritch Awaiting requested review from ritch

Assignees

@bajtos bajtos

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bajtos @ritch @superkhau