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

Fix tests after removing 2.x in workspace #405

Merged
merged 1 commit into from
May 13, 2019
Merged

Fix tests after removing 2.x in workspace #405

merged 1 commit into from
May 13, 2019

Conversation

dhmlau
Copy link
Member

@dhmlau dhmlau commented May 7, 2019

Description

After the PR strongloop/loopback-workspace#549 to remove LB2.x version in the loopback-workspace, there are a few tests failure.

The major changes are in the remote-method.test.js. Following what @bajtos provided in this comment strongloop/loopback-workspace#549 (comment), I've made the changes in this repo accordingly.

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

dhmlau
dhmlau commented May 7, 2019
View reviewed changes
test/remote-method.test.js
@@ -74,10 +73,7 @@ describe('loopback:remote-method generator', function() {
}).then(function() {
var definition = common.readJsonSync('common/models/car.json');
var methods = definition.methods || {};
expect(methods).to.have.property('myRemote');
Copy link
Member Author

Choose a reason for hiding this comment

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

Reading from this docs page: https://loopback.io/doc/en/lb3/3.0-Release-Notes.html#name-indicates-whether-method-is-static, I think this line and the line below are no longer true. But I'm not 100% sure.
@bajtos @jannyHou @hacksparrow, I'd like to get your inputs on this. If this is true, perhaps we need to change the test name as well.

Test name method name with 'prototype.' should be removed -> method name with 'prototype.' should not removed?

Copy link
Member

Choose a reason for hiding this comment

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

Your fix looks correct to me 👍 I agree to update the test name to match the actual assertions.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dhmlau Fair enough 💯

@dhmlau dhmlau self-assigned this May 8, 2019
jannyHou
jannyHou requested changes May 9, 2019
View reviewed changes
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

@dhmlau The travis test has 18 failures, I would suggest fixing them from removing the option loopbackVersion:

generator-loopback/app/index.js

Line 51 in 2241fa0

this.option('loopbackVersion', {

And its related code.

@dhmlau dhmlau mentioned this pull request May 9, 2019
Merged
2 tasks
@dhmlau
Copy link
Member Author

dhmlau commented May 9, 2019

I've discussed with @jannyHou about removing the prompt for loopbackVersion. This might affect the APIC toolkit automated testing, so it might be safer not to remove it at this point, and she agreed.

@dhmlau dhmlau mentioned this pull request May 10, 2019
Merged
1 task
@bajtos
Copy link
Member

bajtos commented May 10, 2019

I've discussed with @jannyHou about removing the prompt for loopbackVersion. This might affect the APIC toolkit automated testing, so it might be safer not to remove it at this point, and she agreed.

From what I remember, APIC used to have it's own version prompt, because they want to use a different default version (2.x) than the one offered by lb app (3.x). I think it's worth checking whether it's still the case.

@dhmlau
Copy link
Member Author

dhmlau commented May 13, 2019

Installed the latest apiconnect (v4.0.24) module, and there is no more prompt of the versions:

$ apic loopback
? Please review the license for API Connect available in /Users/dianalau/.nvm/versions/node/v8.12.0/lib/node? Please review the license for API Connect available in /Users/dianalau/.nvm/versions/node/v8.12.0/lib/node_modules/apiconnect/node_modules/apiconnect-cli-plugins/LICENSE.txt and select yes to accept. yes
? Help us help you!  By providing the API Connect customer excellence team with
limited usage information about your product experience, we can pro-actively
provide assistance to enable your success while continuing to enhance the
product based on your feedback.  If you don't like it, you can always disable
it from My Account > Live Help
 no
? What's the name of your application? apic-lb3-app
? Enter name of the directory to contain the project: apic-lb3-app
? What kind of application do you have in mind? hello-world
...

@jannyHou
Copy link
Contributor

@dhmlau I remember I removed the loopback version prompt in APIC v5. Let me double check.

@jannyHou
Copy link
Contributor

Yes it's removed in apic, see https://github.ibm.com/apimesh/apiconnect-cli-loopback/blob/master/lib/app-prompt.js#L65 the version can only be 3.x and I removed the prompt from the CLI.

jannyHou
jannyHou approved these changes May 13, 2019
View reviewed changes
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@dhmlau
Copy link
Member Author

dhmlau commented May 13, 2019

Will submit a PR in loopback-cli to fix the CI failure.

@dhmlau dhmlau merged commit d32f511 into master May 13, 2019
@dhmlau dhmlau deleted the fix-tests branch May 13, 2019 21:34
@dhmlau dhmlau mentioned this pull request May 14, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jannyHou jannyHou jannyHou approved these changes

@bajtos bajtos Awaiting requested review from bajtos

@hacksparrow hacksparrow Awaiting requested review from hacksparrow hacksparrow is a code owner

@rashmihunt rashmihunt Awaiting requested review from rashmihunt rashmihunt is a code owner

Assignees

@dhmlau dhmlau

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dhmlau @bajtos @jannyHou