Skip to content

fix: errors in mock oauth2 app export from test folder #5386

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
Merged
deepakrkris merged 1 commit into from
May 18, 2020
Merged

fix: errors in mock oauth2 app export from test folder #5386

deepakrkris merged 1 commit into from
May 18, 2020

Conversation

@deepakrkris
Copy link
Contributor

@deepakrkris deepakrkris commented May 11, 2020 •
edited
Loading

fixes: #5380

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • 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

👉 Check out how to submit a PR 👈

@deepakrkris deepakrkris mentioned this pull request May 11, 2020
Closed
raymondfeng
raymondfeng reviewed May 11, 2020
View reviewed changes
extensions/authentication-passport/src/mock-oauth2-social-app.ts
'use strict';

import bodyParser from 'body-parser';
import express from 'express';
Copy link
Contributor

@raymondfeng raymondfeng May 11, 2020

Choose a reason for hiding this comment

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

It might be better to have it under src/fixtures/.

@raymondfeng
Copy link
Contributor

raymondfeng commented May 11, 2020

Maybe we should find a better way to share the mock application, such as its package. Exporting a mock app from the @loopback/authentication-passport module is anti-pattern for LoopBack packages.

@deepakrkris
Copy link
Contributor Author

deepakrkris commented May 11, 2020

authentication-passport

@raymondfeng authentication-passport is not a core package, so I think this is fine atleast for now. But there is a story to migrate loopback-component-oauth2 for which I can add some features and move this into it's own package.

@raymondfeng
Copy link
Contributor

raymondfeng commented May 11, 2020

@raymondfeng authentication-passport is not a core package, so I think this is fine atleast for now. But there is a story to migrate loopback-component-oauth2 for which I can add some features and move this into it's own package.

The extension packages follow the same convention as core ones for consistency.

@bajtos What do you think?

@bajtos
Copy link
Member

bajtos commented May 12, 2020

I agree with Raymond that it's a bad practice for packages to export test fixtures. Ideally, we should be excluding the entire dist/__tests__ directory from our npm packages.

What are you @deepakrkris trying to achieve here? Why is authentication-passport exporting a mock application?

If you are trying to share test setup between multiple packages, then please follow the approach we are using for our repository-related code:

  • packages/repository is providing the runtime behavior
  • packages/repository-tests is exporting a shared test suite. It also comes with a local test suite that's running the shared test suite against the memory connector
  • acceptance/repository-mysql and other packages are running the shared test suite against different connectors

In your case, perhaps you should create a new (internal/private?) package providing the mock application, and then add it to dev-dependencies of your projects.

Thoughts?

@bajtos
Copy link
Member

bajtos commented May 12, 2020

Having wrote the above, maybe it's ok to land the changes in this PR as a short-term fix and then follow up with a refactoring to remove the mock app from exports.

@raymondfeng
Copy link
Contributor

raymondfeng commented May 12, 2020 •
edited
Loading

In your case, perhaps you should create a new (internal/private?) package providing the mock application, and then add it to dev-dependencies of your projects.

I like the idea. Let's add a fixtures top-level directory under loopback-next and configure lerna.json to add the new location. We can move the mock app to be under that and declare devDepedencies for its consumer packages.

Or simply use /acceptance for now.

@raymondfeng
Copy link
Contributor

raymondfeng commented May 12, 2020

@deepakrkris Let me know if you need some help to set it up.

@deepakrkris
Copy link
Contributor Author

deepakrkris commented May 12, 2020

I am going to implement comments from @bajtos and @raymondfeng , then submit this PR for review again.

@deepakrkris deepakrkris requested a review from bajtos as a code owner May 12, 2020 22:35
@deepakrkris deepakrkris force-pushed the fixMockAppExport branch 8 times, most recently from 0643e9e to 201a806 Compare May 13, 2020 00:40
@deepakrkris
Copy link
Contributor Author

deepakrkris commented May 13, 2020

@bajtos @raymondfeng please review

raymondfeng
raymondfeng reviewed May 13, 2020
View reviewed changes
extensions/authentication-passport/tsconfig.json Outdated
"include": [
"src"
"src",
"../../fixtures/mock-oauth2-provider/src/mock-oauth2-social-app.ts"
Copy link
Contributor

@raymondfeng raymondfeng May 13, 2020

Choose a reason for hiding this comment

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

This is wrong. We should list @loopback/mock-oauth2-provider as devDependency.

Copy link
Contributor Author

@deepakrkris deepakrkris May 14, 2020

Choose a reason for hiding this comment

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

agreed

raymondfeng
raymondfeng reviewed May 13, 2020
View reviewed changes
fixtures/mock-oauth2-provider/LICENSE Outdated
@@ -0,0 +1,25 @@
Copyright (c) IBM Corp. 2017,2019.
Copy link
Contributor

@raymondfeng raymondfeng May 13, 2020

Choose a reason for hiding this comment

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

Please fix the header - I believe the year should be 2020.

Copy link
Contributor Author

@deepakrkris deepakrkris May 14, 2020

Choose a reason for hiding this comment

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

agreed

raymondfeng
raymondfeng reviewed May 13, 2020
View reviewed changes
fixtures/mock-oauth2-provider/package.json Outdated
{
"name": "@loopback/mock-oauth2-provider",
"version": "0.0.1",
"description": "",
Copy link
Contributor

@raymondfeng raymondfeng May 13, 2020

Choose a reason for hiding this comment

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

Add some description?

Copy link
Contributor Author

@deepakrkris deepakrkris May 14, 2020

Choose a reason for hiding this comment

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

agreed

raymondfeng
raymondfeng reviewed May 13, 2020
View reviewed changes
fixtures/mock-oauth2-provider/package.json Outdated
"dependencies": {
"passport": "^0.4.1",
"tslib": "^1.11.2",
"util-promisifyall": "^1.0.6"
Copy link
Contributor

@raymondfeng raymondfeng May 13, 2020

Choose a reason for hiding this comment

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

Is it used somewhere in the code?

Copy link
Contributor Author

@deepakrkris deepakrkris May 14, 2020

Choose a reason for hiding this comment

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

agreed, will remove

raymondfeng
raymondfeng reviewed May 13, 2020
View reviewed changes
fixtures/mock-oauth2-provider/package.json Outdated
"devDependencies": {
"@loopback/build": "^5.3.1",
"@loopback/eslint-config": "^6.0.6",
"tslib": "^1.11.2",
Copy link
Contributor

@raymondfeng raymondfeng May 13, 2020

Choose a reason for hiding this comment

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

This is a duplicate of regular dep.

Copy link
Contributor Author

@deepakrkris deepakrkris May 14, 2020

Choose a reason for hiding this comment

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

agreed

raymondfeng
raymondfeng reviewed May 13, 2020
View reviewed changes
fixtures/mock-oauth2-provider/package.json Outdated
"jsonwebtoken": "^8.5.1",
"lodash": "^4.17.15",
"passport-http": "^0.3.0",
"passport-oauth2": "^1.5.0"
Copy link
Contributor

@raymondfeng raymondfeng May 13, 2020

Choose a reason for hiding this comment

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

I believe some of the devDependencies should be moved into dependencies if they are used in the code of the mock app (not tests).

Copy link
Contributor Author

@deepakrkris deepakrkris May 14, 2020

Choose a reason for hiding this comment

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

agreed

raymondfeng
raymondfeng reviewed May 14, 2020
View reviewed changes
fixtures/mock-oauth2-provider/package.json Outdated
"devDependencies": {
"@loopback/build": "^5.3.1",
"@loopback/eslint-config": "^6.0.6",
"tslib": "^1.11.2",
Copy link
Contributor

@raymondfeng raymondfeng May 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This needs to be in dependencies only.

Copy link
Contributor Author

@deepakrkris deepakrkris May 14, 2020

Choose a reason for hiding this comment

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

agreed

raymondfeng
raymondfeng reviewed May 14, 2020
View reviewed changes
fixtures/mock-oauth2-provider/package.json Outdated
},
"dependencies": {
"passport": "^0.4.1",
"tslib": "^1.11.2",
Copy link
Contributor

@raymondfeng raymondfeng May 14, 2020

Choose a reason for hiding this comment

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

The latest version should be 2.0.0 now.

Copy link
Contributor Author

@deepakrkris deepakrkris May 14, 2020

Choose a reason for hiding this comment

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

@raymondfeng all loopback packages refer only tslib 1.12.0 as of now

Copy link
Contributor

@raymondfeng raymondfeng May 14, 2020

Choose a reason for hiding this comment

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

No, you need to rebase to master. It's all 2.0.0 now.

raymondfeng
raymondfeng reviewed May 14, 2020
View reviewed changes
fixtures/mock-oauth2-provider/package.json Outdated
"dependencies": {
"passport": "^0.4.1",
"tslib": "^1.11.2",
"tslib": "^1.12.0",
Copy link
Contributor

@raymondfeng raymondfeng May 14, 2020

Choose a reason for hiding this comment

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

The version should be 2.0.0.

@deepakrkris deepakrkris force-pushed the fixMockAppExport branch 2 times, most recently from f7c4b7e to b8862b2 Compare May 14, 2020 21:59
@raymondfeng
Copy link
Contributor

raymondfeng commented May 15, 2020

@deepakrkris Please squash the commits into one before landing it.

bajtos
bajtos suggested changes May 15, 2020
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Great, this new version looks much better now!

Please register the new package in the following places, as described in https://github.com/strongloop/loopback-next/blob/master/docs/site/DEVELOPING.md#adding-a-new-package:

  • Update MONOREPO.md - insert a new table row to describe the new package, please keep the rows sorted by package name.
  • Update CODEOWNERS - add a new entry listing the primary maintainers (owners) of the new package.

bajtos
bajtos approved these changes May 18, 2020
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Almost there! Please move the new row in MONOREPO.md in such place that the table stays sorted.

docs/site/MONOREPO.md Outdated
| [service-proxy](https://github.com/strongloop/loopback-next/tree/master/packages/service-proxy) | @loopback/service-proxy | A common set of interfaces for interacting with service oriented backends such as REST APIs, SOAP Web Services, and gRPC microservices |
| [testlab](https://github.com/strongloop/loopback-next/tree/master/packages/testlab) | @loopback/testlab | A collection of test utilities we use to write LoopBack tests |
| [tsdocs](https://github.com/strongloop/loopback-next/tree/master/packages/tsdocs) | @loopback/tsdocs | An internal package to generate api docs using Microsoft api-extractor and api-documenter |
| [fixtures/mock-oauth2-provider](https://github.com/strongloop/loopback-next/tree/master/fixtures/mock-oauth2-provider) | _(private)_ | An internal app to mock the OAuth2 authorization flow login with a social app like facebook, google etc |
Copy link
Member

@bajtos bajtos May 18, 2020

Choose a reason for hiding this comment

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

See https://github.com/strongloop/loopback-next/blob/1083430cee68d11b6328d32e285e02afe4550c1c/docs/site/MONOREPO.md#L6

<!-- PLEASE KEEP THE TABLE ROWS SORTED ALPHABETICALLY BY PACKAGE NAME-->

@deepakrkris deepakrkris merged commit 5c4d10f into master May 18, 2020
@deepakrkris deepakrkris deleted the fixMockAppExport branch May 18, 2020 18:43
@bajtos bajtos mentioned this pull request May 22, 2020
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@raymondfeng raymondfeng raymondfeng approved these changes

@emonddr emonddr Awaiting requested review from emonddr

@hacksparrow hacksparrow Awaiting requested review from hacksparrow

@jannyHou jannyHou Awaiting requested review from jannyHou

+1 more reviewer

@bajtos bajtos bajtos approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Authentication passport] tests in an index file?

4 participants

@deepakrkris @raymondfeng @bajtos