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

Drop overly-complected Christmas tree tests #145

Merged
merged 4 commits into from
Dec 9, 2020
Merged

Conversation

vinceatbluelabs
Copy link
Contributor

@vinceatbluelabs vinceatbluelabs commented Dec 9, 2020

To increase test coverage around part of the code which turn hints into config for various tools, I had used a test pattern I now regret. This PR removes tests which use that pattern.

image

self.assertListEqual(mock_warning.mock_calls,
[call("Ignoring hint compression = 'LZO'"),
call("Ignoring hint quoting = 'nonnumeric'")])
self.assertEqual(unhandled_hints, set())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests try to validate things by a growing collection of eclectic hints which try to test a number of different things simultaneously.

Cool, I guess, but the downside is that debugging them when they fail is really hard and creates cognitive load.

Instead I'd like to test hints more individually - the tests in #144 and #140 are better examples of how I'd like to write these tests going forward.

I've been wanting to kill these for a while, but the forcing function was that I tried to debug these after making clearly safe changes and it gave me too much of a head-ache trying to detangle what was going on.

Copy link

@crvena-sonja crvena-sonja left a comment

Choose a reason for hiding this comment

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

We're throwing the tree out rather early, but thank you for taking pity on those of us who would have tried to debug this later.... 😬

:shipit:

@@ -1 +1 @@
93.6700
93.1600
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the downside. I'll make up for some of that in #140, for sure, but won't have time to get us back to this number.

Choose a reason for hiding this comment

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

I do think that the complexity-reduction here is an overall benefit.

@vinceatbluelabs vinceatbluelabs marked this pull request as ready for review December 9, 2020 20:05
@vinceatbluelabs vinceatbluelabs merged commit cc22521 into master Dec 9, 2020
@ryantimjohn ryantimjohn deleted the grinch branch December 17, 2022 11:14
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.

2 participants