Skip to content

Conversation

@zackgalbreath
Copy link
Collaborator

This test currently only verifies that the TrackerDash consists of a single parent element.

WIP.  This test currently only verifies that the TrackerDash consists
of a single parent element.
@zackgalbreath
Copy link
Collaborator Author

This buys us a pretty good bump in coverage, but this test (as currently written) doesn't do much to protect the behavior of this component.

Thoughts on how to proceed? As I see it, we have two options here:

  1. Instead of instantiating TrackerDash directly, build up coverage by testing its individual sub-components instead.

  2. Accept this big bump of coverage and gradually try to add more predictive test cases for TrackerDash, even though they won't result in any additional coverage.

@jeffbaumes
Copy link
Contributor

Instead of performing a full test of each component, we could dig in to a few of the sub-components in the TrackerDash DOM with this test. To help the process, you could inspect the TrackerDash example to see what the HTML looks like, then update the test to examine some of the children (e.g. are the right number of bullet charts created? is the summary plot there? do some of the data-driven headings have the correct text?)

@waxlamp
Copy link
Member

waxlamp commented Jun 15, 2017

@zackgalbreath, what do you think of @jeffbaumes's suggestion?

The changes in this PR look good to me, but we have to decide how to go forward with the testing strategy itself.

@zackgalbreath zackgalbreath changed the title [WIP] Add rudimentary unit test for TrackerDash Add rudimentary unit test for TrackerDash Jun 19, 2017
@mgrauer
Copy link
Contributor

mgrauer commented Jun 19, 2017

I'd favor setting up the testing skeleton with one or two pieces of meat, then saving the rest of the fill in for later. Once the skeleton is in place, increasing coverage could be a good learning opportunity for someone new to Candela.

@waxlamp
Copy link
Member

waxlamp commented Oct 19, 2017

@zackgalbreath, should we go ahead and merge this now, and decide how to proceed more fully later?

Sorry to revive an old thread but we should definitely do something with this PR :)

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.

4 participants