feat(codecov): Add output coverage file step 1 #92044
Conversation
github-actions
bot
added
the
Scope: Frontend
calvin-codecov
changed the title
| import {OnboardingStep} from 'sentry/views/codecov/tests/onboardingSteps/onboardingStep'; | ||
|
|
||
| interface OutputCoverageFileProps { | ||
| stepString: string; |
There was a problem hiding this comment.
Nit: adding the type of variable to the name itself is kind of redundant when you have it typed by a type. I'd call this step or something and cast the number to str if you're expecting that to be a number
There was a problem hiding this comment.
The steps could include a sub-step letter like "2a" so it will need to stay a string.
| supported: ( | ||
| // TODO: the new version of this link is still TBD | ||
| <Link to="https://docs.codecov.com/docs/test-analytics#:~:text=Only%20JUnit%20XML%20test%20result%20files%20are%20supported%20at%20the%20moment"> | ||
| supported languages |
| <CodeSnippet dark language="bash"> | ||
| {INSTALL_REQUIREMENTS_SNIPPETS[selectedFramework]} | ||
| </CodeSnippet> | ||
| {GENERATE_FILE_SNIPPETS[selectedFramework] ? ( |
There was a problem hiding this comment.
Thoughts on just doing an equivalence with selectedFramework === "pytest"? Feels we're only creating that object to have this case for pytest and the rest are defaulted to nothing. SelectedFramework should be typed so that works too. We could expand that later if other frameworks need this behavior.
Also we could do {A && } rather than a ternary to avoid the null
There was a problem hiding this comment.
Sure thing, I'll change it to just check for "pytest". Yeah, it's only for pytest at the moment.
Re {A &&}: in gazebo, we've been following a pattern to use ternary with null instead of && statements to avoid accidental rendering of non-undefined falsy values like 0, false, or an empty string so I decided to continue the pattern
There was a problem hiding this comment.
Fair fair, I agree w/ that, it ultimately is more explicit/readable so i support that
| margin-bottom: ${space(1.5)}; | ||
| `; | ||
|
|
||
| const StyledP = styled('p')` |
There was a problem hiding this comment.
Nit: naming in case you find something more descriptive, naming is hard 😂
| /* flex-grow: 1; */ | ||
| `; | ||
|
|
||
| export const OnboardingStep = { |
There was a problem hiding this comment.
Curious why you decided to lump these 3 styled components into this file. It feels like we're trying to think this as a component rather than at markup so trying to get a feel of why it's on it's own file.
There was a problem hiding this comment.
I was going to reuse this composition components for all of the steps so they wouldn't be associated with one specific file/component
There was a problem hiding this comment.
Ahh fair fair I see. Do you ever foresee these steps having different styling bw their headers, contents or containers? I'd normally say this is preemptive abstraction but I think there's a valid case to say you want all of them to be the same, so this comes as a handy way for that if that's how we foresee this behaving in the future. I trust your judgement here 👍
There was a problem hiding this comment.
Cool, I will leave it for now then. I do think there's a possibility for steps to have different styling but looking at our current designs, they are all relatively identical. If we run into an outlier, we can explicitly define new styles to be used instead of the styled composition components for that/those instance(s).
calvin-codecov
force-pushed
the
cy/step1_codecov_onboarding
branch
from
f00ca17 to
6b6f2e7
Compare
Closes https://linear.app/getsentry/issue/CCMRG-427/step-1-outputting-coverage-file-in-ci
Adds step 1 of TA onboarding
Preview: https://sentry-2ag4w6q3q.sentry.dev/codecov/tests/new