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

Re-enable energy unit test #86

Closed
wants to merge 2 commits into from
Closed

Re-enable energy unit test #86

wants to merge 2 commits into from

Conversation

jayaddison
Copy link

Re-enables the skipped 'energy' sankey test; this requires updating some of the test expectations.

@jayaddison
Copy link
Author

Does this look reasonable @curran @Fil @mbostock?

I'd like to merge this (and #87) before adding test coverage to #71.

(apologies for the multi-ping; if there's a better way to ping for d3 reviews, and/or if I should just wait in future, please let me know :))

@Fil
Copy link
Member

Fil commented Nov 18, 2020

Well I've never contributed to this repo (and never used it), so I have to pass :)
But adding or fixing a test is (almost) always a good idea.

@jayaddison
Copy link
Author

No probs, thanks :)

@jayaddison
Copy link
Author

Ping @mbostock; this pull request re-enables a unit test for d3-sankey.

@jayaddison
Copy link
Author

Just checking in again to see whether we could re-enable this unit test; this would unblock continued progress on #71.

Copy link

@curran curran left a comment

Choose a reason for hiding this comment

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

LGTM

@jayaddison
Copy link
Author

ping @mbostock (after merging this and #87, I plan to add test coverage to #71, per review feedback there)

@jayaddison
Copy link
Author

@mbostock any chance of review on this change to re-enable a unit test (with updated test expectations)? I'd like to do that before adding test coverage in #71 (vertical sankey orientation support).

@jayaddison jayaddison closed this Mar 30, 2022
@jayaddison jayaddison deleted the enable-energy-test branch March 30, 2022 19:50
@jayaddison jayaddison mentioned this pull request May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants