Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Improving unit test coverage for simpler cases #117

Merged
merged 2 commits into from
Apr 19, 2020
Merged

Improving unit test coverage for simpler cases #117

merged 2 commits into from
Apr 19, 2020

Conversation

kumare3
Copy link
Contributor

@kumare3 kumare3 commented Apr 19, 2020

TL;DR

We want to drive overall unit test coverage. Following this checkin we will enforce strict coverage gates

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Tracking Issue

NA

Follow-up issue

NA

@kumare3 kumare3 requested a review from EngHabu as a code owner April 19, 2020 15:56
Copy link
Contributor

@wild-endeavor wild-endeavor left a comment

Choose a reason for hiding this comment

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

Approving. I'm personally okay with lower test coverage. Sure, referentially transparent functions should have unit tests, but I feel like none of the issues we've seen recently would've been caught by any of these tests. They would've probably benefited more from better integration testing and better staging-traffic live testing, load testing, etc. so I wonder if our time isn't better spent in those areas. Oftentimes I feel like I'm forced to write inane unit tests just to get the numbers up.

@kumare3
Copy link
Contributor Author

kumare3 commented Apr 19, 2020

@wild-endeavor unit tests are not always for protecting large problems in systems, IMO they serve 2 purposes

  1. When you write unit tests you realize if the API (for the function) you have designed, whether it is usable, do you have the right assumptions and sometimes, they serve as a documentation
  2. It gives new contributors confidence to change code, knowing that a breakage in the unit test should result in case they make some change that breaks existing assumptions.

@kumare3 kumare3 merged commit 64dd8ed into master Apr 19, 2020
kumare3 pushed a commit to nuclyde-io/flytepropeller that referenced this pull request Feb 4, 2021
In case there is no override in resource table, we should allow the code
to go through and apply non-label based weighted random selection.

Close flyteorg/flyte#481
eapolinario pushed a commit to eapolinario/flytepropeller that referenced this pull request Aug 9, 2023
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.

2 participants