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

[Lubuild] fix multiple bugs: case sensitive issue, default version issue and exception not throw out issue #576

Merged
merged 30 commits into from Feb 27, 2020

Conversation

feich-ms
Copy link
Contributor

@feich-ms feich-ms commented Feb 17, 2020

Fix several bugs:

  1. As known to us, values of utterances/patterns are automatically converted to lower case in luis portal even though typed words are upper case. When comparing app json from lu file with the one from remote existing luis app, we should to ignore cases. To achieve this, I converted the objects to string, then to lower case so that the comparison works insensitively.
  2. Some properties in our luis definition don't align with the object pulled from luis portal. Fixed in this PR
  3. Default version 0.1 was included by previous PR when no version is specified which caused some bugs when updating version. In this PR, version from lu will count only if it's value is over than existing app. For example, if the app with the same app id in luis portal is version 0.1, and the version from the lu file is 0.3, 0.3 > 0.1, then 0.3 will count, otherwise, 0.1 will auto increase by 0.1 to 0.2
  4. Add test cases for above bug fixes
  5. Add test cases for luis:build refinements #548
  6. Fix bug Exceptions are not returned properly in luis:build #582

@feich-ms feich-ms changed the title [Lubuild] fix case sensitive issue when comparing luis applications json [Lubuild] fix case sensitive issue when comparing luis applications json to update Feb 17, 2020
Copy link
Contributor

@vishwacsena vishwacsena left a comment

Choose a reason for hiding this comment

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

Can you please add tests as well?

@feich-ms
Copy link
Contributor Author

@vishwacsena, sure, will add tests soon.

@feich-ms
Copy link
Contributor Author

Tests added @vishwacsena

Copy link
Contributor

@vishwacsena vishwacsena left a comment

Choose a reason for hiding this comment

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

@munozemilio it might make sense to expose compareApps as a public method on LUIS?

Copy link
Member

@munozemilio munozemilio left a comment

Choose a reason for hiding this comment

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

CIL

@feich-ms
Copy link
Contributor Author

@munozemilio, comments resolved. Also added test cases for luConfig support of #548. Fixed bug of #582 as well.

@feich-ms feich-ms changed the title [Lubuild] fix case sensitive issue when comparing luis applications json to update [Lubuild] fix multiple bugs: case sensitive issue, default version issue and exception not throw out issue Feb 19, 2020
Copy link
Member

@munozemilio munozemilio left a comment

Choose a reason for hiding this comment

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

Some comments in line

@feich-ms
Copy link
Contributor Author

Comments resolved. Thanks @munozemilio

@vishwacsena
Copy link
Contributor

@feich-ms I tried this E2E and pushed up two sets of deltas up to your branch. I'm not familiar with nock and some of the lubuild tests are failing. Can you take a look and address?

@vishwacsena
Copy link
Contributor

@munozemilio except for addressing tests, it would be good if you get a review as well. Thanks!

Copy link
Member

@munozemilio munozemilio left a comment

Choose a reason for hiding this comment

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

CIL

@feich-ms
Copy link
Contributor Author

feich-ms commented Feb 24, 2020

@munozemilio, tests failures are fixed. Please help to approve. Thanks.

The merge is blocked by 'Request to change'. Could you approve if there are no other issues.

@feich-ms
Copy link
Contributor Author

feich-ms commented Feb 27, 2020

@munozemilio, I added more modules in index.js and composerindex.js in the latest commit. My intention is to expose modules that composer client/broswer consumes in composerindex.js and other modules in index.js. Parser and SectionHandler is consumed by composer client and luBuild is consumed by composer server. Please let me know if this is not the reasonable way. Thanks.

Copy link
Member

@munozemilio munozemilio left a comment

Choose a reason for hiding this comment

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

Approved with a comment

@feich-ms feich-ms merged commit 49f1871 into master Feb 27, 2020
@feich-ms feich-ms deleted the feich/fixLubuildBug branch February 27, 2020 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants