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

[ZEPPELIN-2778] Unit test for credential module (zeppelin-web) #2493

Conversation

1ambda
Copy link
Member

@1ambda 1ambda commented Jul 14, 2017

What is this PR for?

Added few test cases for the credential module under zeppelin-web/

Additionally,

  • modified configuration.html to add the missing tbody.
  • removed useless lodash dependency
  • extract some funcs to reuse them.

What type of PR is it?

[Improvement]

What is the Jira issue?

ZEPPELIN-2778

How should this be tested?

  1. cd zeppelin-web
  2. yarn install (or npm install)
  3. yarn run test (or npm run test)

The test should pass.

Questions:

  • Does the licenses files need update? - NO
  • Is there breaking changes for older versions? - NO
  • Does this needs documentation? - NO

Copy link
Contributor

@sravan-s sravan-s left a comment

Choose a reason for hiding this comment

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

LGTM, Worked on local
Apart from some nitpicking in code(which doesn't matter), everything looks good to go 👍

.success(function (data, status, headers, config) {
$scope.credentialInfo = _.map(data.body.userCredentials, function (value, prop) {
return {entity: prop, password: value.password, username: value.username}
.success(function (data, status, headers, config) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@1ambda, In Angular 1.6, Promise.success has been removed. https://github.com/angular/angular.js/blob/master/CHANGELOG.md#160-rainbow-tsunami-2016-12-08

Even though Zeppelin uses 1.5, it's good to be aware of this

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the information, we can solve that issue in ZEPPELIN-2788

@1ambda
Copy link
Member Author

1ambda commented Jul 20, 2017

Thanks for the review @sravan-s

Merge if no more discussion

@asfgit asfgit closed this in ca40e49 Jul 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants