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-2779] Unit test for job module (zeppelin-web) #2497

Closed

Conversation

1ambda
Copy link
Member

@1ambda 1ambda commented Jul 19, 2017

What is this PR for?

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

Additionally,

  • removed lodash, q dependency
  • converted to JobModule
  • refactored jobmanager/*

What type of PR is it?

[Improvement]

What is the Jira issue?

ZEPPELIN-2779

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

@1ambda
Copy link
Member Author

1ambda commented Jul 20, 2017

@sravan-s Please help review this PR.

if (!clickOk) { return }

this.sendRunJobRequest()
.catch(response => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have any sucess messages?

Copy link
Member Author

Choose a reason for hiding this comment

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

Job page gets updated by websocket when the requested job is done.

Copy link
Member Author

Choose a reason for hiding this comment

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

added comment

expect(progress1).toBe('50%')
})

it('should get proper job type icons', () => {
Copy link
Contributor

@sravan-s sravan-s Jul 20, 2017

Choose a reason for hiding this comment

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

test description (should get proper job type icons) is duplicated in this one and the next one

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed the duplicated test

return acc
}, {})

let notes = responseData.jobs
Copy link
Contributor

Choose a reason for hiding this comment

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

I scrolled down and saw this is from jobmanager.controller.js. @1ambda, what do you think? Is it a good idea to change the code here?

IMO, The logic here seems a bit complicated. A lot of side effects are happening inside notes.map
It would be helpful, if we can simplify the logic or have a small one line comment describing what's happening

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's keep that part :)

@1ambda 1ambda force-pushed the ZEPPELIN-2779/add-unit-test-for-job-page branch from 3e7a3b3 to ae3b79a Compare July 21, 2017 16:24
@1ambda
Copy link
Member Author

1ambda commented Jul 21, 2017

@sravan-s I updated the PR, now websocket (un)subscription is handled in this way (2e12c21)

// job.component.js
  function init() {
    JobManagerService.getJobs()
    JobManagerService.subscribeSetJobs($scope, setJobsCallback)
    JobManagerService.subscribeUpdateJobs($scope, updateJobsCallback)

    $scope.$on('$destroy', function () {
      JobManagerService.disconnect()
    })

Here are logs for it.

image

@sravan-s
Copy link
Contributor

Looks interesting, let me see :)

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

@1ambda 1ambda closed this Jul 22, 2017
@1ambda 1ambda reopened this Jul 22, 2017
@1ambda
Copy link
Member Author

1ambda commented Jul 24, 2017

Thanks for the review @sravan-s

Merge if no more discussion.

@asfgit asfgit closed this in 90b3be5 Jul 25, 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