Skip to content

Conversation

@lklgithub
Copy link

No description provided.

@jingwang315
Copy link

Good points:

  1. baby step commit
  2. meaningful commit message and naming
  3. component covered by test
  4. no magic code

expect(component.counters.length).toEqual(4)
});

it('should remove counters when call OnResetCounters', () => {

Choose a reason for hiding this comment

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

suggestion: test description is misleading, what we test here is the remove feature

@@ -0,0 +1,11 @@
<button (click)="OnAdd()">add counter</button>

Choose a reason for hiding this comment

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

suggestion: missing title "Counter Group", it should be on top of the "add counter" button

{num: 3}
]
component.removeCounter(1)
expect(component.counters.length).toEqual(2)

Choose a reason for hiding this comment

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

well: nice to verify counters when test remove feature

this.counters.push({num: 0})
}

OnResetCounters() {

Choose a reason for hiding this comment

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

suggestion: this method should also be covered by test

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.

3 participants