Skip to content
This repository was archived by the owner on Apr 30, 2018. It is now read-only.

Conversation

MCKRUZ
Copy link

@MCKRUZ MCKRUZ commented Nov 9, 2015

Fix for deeply nested variables not getting watchers setup properly

Fix for deepl nested variables not getting watchers setup properly
@codecov-io
Copy link

Current coverage is 95.92%

Merging #545 into master will not affect coverage as of ffff26c

@@            master    #545   diff @@
======================================
  Files           16      16       
  Stmts         1103    1103       
  Branches         0       0       
  Methods          0       0       
======================================
  Hit           1058    1058       
  Partial          0       0       
  Missed          45      45       

Review entire Coverage Diff as of ffff26c


Uncovered Suggestions

  1. +0.36% via ...alidationMessages.js#25...28
  2. +0.27% via src/test.utils.js#38...40
  3. +0.27% via ...s/formlyUsability.js#18...20
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

const deepLinkField = getNewField()
deepLinkField.key = 'foo.bar'
deepLinkField.watcher = {
listener: '',
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this test is testing what you think it is... I think you're getting code coverage, but the watchExpression here never gets evaluated. Could you add a new test that has a deepLink field like this except for the expression uses a spy and asserts that the spy was called properly?

Copy link
Author

Choose a reason for hiding this comment

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

Before I recommit.

Does this work for you?

it(should setup any watchers specified on a field, () => {
scope.model = {}

  const listener = sinon.spy()
  const expression = sinon.spy()
  const deepLinkField = getNewField()
  deepLinkField.key = 'foo.bar'
  deepLinkField.watcher = {
    listener: sinon.spy(),
    expression: sinon.spy(),
  }

  scope.fields = [getNewField({
    watcher: {
      listener: '',
    },
  }),
  deepLinkField, getNewField({
    watcher: [{
      listener: '',
      expression: '',
    }, {
      listener,
      expression,
    }],
  })]

  expect(compileAndDigest).to.not.throw()
  expect(listener).to.have.been.called
  expect(expression).to.have.been.called
  expect(deepLinkField.watcher.listener).to.have.been.called
  expect(deepLinkField.watcher.expression).to.have.been.called
})

})

From: "Kent C. Dodds" notifications@github.com
Reply-To: formly-js/angular-formly reply@reply.github.com
Date: Monday, November 9, 2015 at 5:24 PM
To: formly-js/angular-formly angular-formly@noreply.github.com
Cc: Matt Kruczek kruz79@gmail.com
Subject: Re: [angular-formly] fix(formly-form): Issue number 542 (#545)

In src/directives/formly-form.test.js:

@@ -933,12 +933,19 @@ describe('formly-form', () => {

   const listener = sinon.spy()
   const expression = sinon.spy()
  •  const deepLinkField = getNewField()
    
  •  deepLinkField.key = 'foo.bar'
    
  •  deepLinkField.watcher = {
    
  •    listener: '',
    
    I don't think that this test is testing what you think it is... I think you're getting code coverage, but the watchExpression here never gets evaluated. Could you add a new test that has a deepLink field like this except for the expression uses a spy and asserts that the spy was called properly?


Reply to this email directly or view it on GitHub.

Copy link
Member

Choose a reason for hiding this comment

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

Almost. We want to test the default listener (which is what you're modifying) so you'd omit the listener on the deepLinkField. Other than that, what you have is sufficient.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants