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

The "Team" plan (FKA Turn off project coverage by default) #132

Closed
8 of 9 tasks
trent-codecov opened this issue Jul 31, 2023 · 15 comments
Closed
8 of 9 tasks

The "Team" plan (FKA Turn off project coverage by default) #132

trent-codecov opened this issue Jul 31, 2023 · 15 comments
Assignees
Labels
epic this label is used to mark issues as epics P0: must do priority 10
Milestone

Comments

@trent-codecov
Copy link
Contributor

trent-codecov commented Jul 31, 2023

KPIs

Applications Tasks

  1. 52 of 52
    epic
    adrian-codecov
  2. 11 of 11
    adrian-codecov

Designs

@trent-codecov trent-codecov added the P0: must do priority 10 label Jul 31, 2023
@trent-codecov trent-codecov added this to the Q3'23 milestone Jul 31, 2023
@katia-sentry katia-sentry added the epic this label is used to mark issues as epics label Aug 2, 2023
@katia-sentry katia-sentry modified the milestone: Q3'23 Aug 3, 2023
@trent-codecov trent-codecov modified the milestones: Q3'23, Q4'23 Aug 3, 2023
@aj-codecov
Copy link

Plan:

  • Small experiment in PR where we ONLY show patch to maybe 5% of customers
    • This requires a redesign of the PR comment and then building it (Dana or maybe Matt on platform)
      • This redesign should include a feedback link on the PR comment itself to give us some knowledge how people feel
      • Do we want to include the thumbs up/down heuristic here on data reliability? Or some iteration of it?
    • Result of this experiement - what is positive? what is success? How do we know this experiment confirms or denies our hypothesis?
  • Based on initial results we can either expand the experiment further OR begin design work on what this could look like in the UI
    • I don’t think it’s realistic to revamp the UI in Q3 if we’re trying to refactor the table component - it’d be heavily impacted/blow out the scope of that refactor dramatically
  • If we decide to move forward with this patch only scenario, I think we could have pricing and UI designs done by end of Q3, but I think having the UI revamped for this is a huge stretch

@adrian-codecov
Copy link

Is this initiative aiming to turn indirect changes off completely, or under some condition? (e.g. some yml setting)

@aj-codecov
Copy link

@adrian-codecov
This will serve as the basis for a whole new plan that's centered around patch only changes - see designs here.

This isn't to say that someday we won't want to give pro and ent plans the ability to turn off project coverage via config, but most of the front end investigation at this point is the new UI in these designs. These are preliminary designs of course, but likely not far from the end result CC @codecovdesign

Only other thing I would add is that we're starting with the PR Comment changes (far left in the designs) and that really shouldn't require much on the app team side to my knowledge.

@adrian-codecov
Copy link

adrian-codecov commented Aug 9, 2023

Gotcha, asking cause I have this investigation, #246, to determine the technical considerations for this whole initiative.

So for this new plan, or potentially extending this to other plans, is optional though right? I'm trying to decipher if I should consider this more of a "we process uploads, get direct + indirect changes, then we filter", or more so of a "we process uploads, only compute the direct changes and not compute the indirect changes". Do you know how this would work?

@jerrodcodecov
Copy link

jerrodcodecov commented Aug 15, 2023

@adrian-codecov can you clarify what is optional in your question?

Most likely, the routing about "patch only" vs. "including project coverage" will be some sort of yaml setting or similar. As such, we should be able to pick up the routing on the fly for the repository in question to understand what to show.

@jerrodcodecov
Copy link

  • I don’t think it’s realistic to revamp the UI in Q3 if we’re trying to refactor the table component - it’d be heavily impacted/blow out the scope of that refactor dramatically

@aj-codecov can you link to the refactor of the table component work? I'm nervous about creating a dependency there

@jerrodcodecov
Copy link

The key piece missing is telling a user how to RE-ENABLE project coverage on PR comment if they so choose (cc: @dana-yaish as well)

  • This requires a redesign of the PR comment and then building it (Dana or maybe Matt on platform)

    • This redesign should include a feedback link on the PR comment itself to give us some knowledge how people feel
    • Do we want to include the thumbs up/down heuristic here on data reliability? Or some iteration of it

Yes, ideally some way to gather qualitative and quantitative feedback would be most ideal. I'd be happy to design something or look at a design.

  • Result of this experiement - what is positive? what is success? How do we know this experiment confirms or denies our hypothesis?

I think the key will be the number of people who decide to RE-ENABLE project coverage after being exposed to patch only coverage.

Again, this is a bit of a reversal from normal Codecov process in the sense that we are starting with the thesis that developers are focused on patch coverage (untested lines in diff, most accurately), and are looking for data to disprove that hypothesis vs. prove it (which is already anecdotally proven)

@jerrodcodecov
Copy link

Some questions from @eliatcodecov and my responses

From
@eli.hooten
questions

  1. We should reach consensus on whether or not we want to disable the project check by default for any user that doesn't have project coverage

Yes, the project status check should be off by default as well (if project coverage is generally set to off). I think it would be sensible to be able to overwrite the project status check to on even while the project coverage was off in PR comment.
Where can I paste that requirement?

  1. After figuring out what to do with the project status check, we could roll this new PR comment that only shows patch coverage out to new users. Essentially we'd just set their default yamls to turn this stuff off, but they could go turn it on in the yaml if they want. This gives us a little bit of an escape hatch if new users just hate this change for some reason

I agree. If so, make sure we link the PR comment to the docs to show how to re-enable project coverage.

@adrian-codecov
Copy link

@jerrodcodecov, I’m trying to figure out how we want/are thinking to process reports in Worker for people that don’t want project coverage (as part of my investigation ticket/focusing on how this would look like on gazebo). I see this being either of two ways:

  1. we process reports the same way we do it today. We process both direct + indirect coverages when a customer uploads a report, and somewhere in our api we filter out what to show in Gazebo (only the direct changes), and Gazebo would react accordingly.

  2. we process reports differently and just process the direct changes. In our api we would only expose that this customer is a “direct-changes only” customer and the client would react accordingly.

I’m not sure if were doing 1) or 2) (or something else), asking here to see if we have clarified that.

If turning off project coverage is optional, I’d lean to do option 1) as we have all data we need computed, but if it is more permanent, I’d lean towards option 2 as it could help speed up how fast we process reports (guessing here, would have to prove with metrics). If we did option 2) and someone retroactively wants to see their indirect changes, we’d have to recompute their upload, which sounds less ideal to me.

Mostly looking for clarity on how we’re intending to process reports for people that don’t want project coverage

@codecovdesign
Copy link
Contributor

@jerrodcodecov, from the thread in Slack:

gather qualitative and quantitative feedback would be most ideal. I'd be happy to design something or look at a design.

I've updated the designs to include a link for feedback collection in this specific issue. It's worth noting this is separate from the new PR comment feedback issue.

Lmk if any other adjustments or updates. Otherwise, created a ticket capturing the update and added it to this epic.

@katia-sentry
Copy link
Contributor

Do we want the project coverage to be optional, or simply hidden from the user's view (without altering the underlying data processing)? This will impact scope and will clarify Adrian's technical direction on which option we want to go with. cc. @eliatcodecov @trent-codecov

@katia-sentry
Copy link
Contributor

@aj-codecov can you link to the refactor of the table component work? I'm nervous about creating a dependency there

We're working on the table refactor this sprint #328. We're creating a new base and then replacing all existing tables in the upcoming months.

@terry-codecov, if we implement changes now to the existing tables, considering the transition to a new base, how much would it complicate or affect our planned switch to the refactored tables?

@jerrodcodecov
Copy link

jerrodcodecov commented Aug 17, 2023

@adrian-codecov seems like, when project coverage: off in the YAML

Pros for computing ALL coverage (including indirect) and then only surfacing direct changes + patch coverage:

-- Allows us to retroactively show project coverage and indirect changes if project coverage: on happens later
-- Therefore gives us more confidence when "testing" this feature because users can toggle back to project coverage: on with no consequence.

Pros for only computing direct coverage changes + patch coverage:

-- Is way faster
-- Is easier to build?

If i understand the trade-offs correctly, then I would vote to only calculate the direct coverage changes + patch coverage when project coverage: off

UPDATE: Nevermind, @eliatcodecov responded on Slack with the opposite response.

It's probably a lot of work to ''undo''
if someone opts to turn project coverage on, then their project coverage history will be present
we may end up receiving feedback that causes us to abandon the idea of disabling project coverage by default so we'll want to make sure we can get back to project coverage displayed by default easily

@codecovdesign
Copy link
Contributor

codecovdesign commented Sep 5, 2023

design review following up on feedback from @jerrodcodecov @aj-codecov, looking at PR comment with the following:

  • Scenario: Ada has modified 17 coverable lines
    • i report shows both the patch % AND missed lined count
    • ii report shows only missed line count

both versions includes different copy variations/ideation for the PR comment focused on 1) emphasizing missing lines ⚠️, 2) confirming covered lines ✅

🎨 review designs

missed_lines_only

@aj-codecov aj-codecov changed the title Turn off project coverage by default The "Team plan (FKA Turn off project coverage by default) Oct 10, 2023
@aj-codecov aj-codecov changed the title The "Team plan (FKA Turn off project coverage by default) The "Team" plan (FKA Turn off project coverage by default) Oct 10, 2023
@katia-sentry
Copy link
Contributor

katia-sentry commented Oct 24, 2023

[Applications] On track for November release

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic this label is used to mark issues as epics P0: must do priority 10
Projects
Status: Done
Development

No branches or pull requests

7 participants