chore(logs): update SDK onboarding pages #104234
Conversation
github-actions
bot
added
the
Scope: Frontend
JoshuaMoelans
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #104234 +/- ##
===========================================
- Coverage 80.60% 80.60% -0.01%
===========================================
Files 9333 9333
Lines 398600 398597 -3
Branches 25497 25496 -1
===========================================
- Hits 321275 321272 -3
Misses 76875 76875
Partials 450 450 |
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
JoshuaMoelans
changed the title
JoshuaMoelans
force-pushed
the
joshua/chore/logs_onboarding
branch
from
8411a74 to
f3a368a
Compare
JoshuaMoelans
requested review from
Flash0ver,
Litarnus,
bitsandfoxes and
priscilawebdev
JoshuaMoelans
force-pushed
the
joshua/chore/logs_onboarding
branch
from
d6b90f6 to
c4e7597
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| '4.3.0' | ||
| '6.0.0' |
There was a problem hiding this comment.
is this updating version for profile? if yes, then perhaps it would be better to have this update in a separate PR
There was a problem hiding this comment.
IIUC the version here is a fallback if getPackageVersion fails, right? I changed the fallback here too because of this comment by Stefan, since we removed the if-elif selection of the fallback version for Logs that I added at first (hence it is slightly related). I'm fine with moving this change into another PR though.
| title: t('Verify Logs'), | ||
| content: [logsVerify(params)], |
There was a problem hiding this comment.
Nit: The current approach works fine as is. However, to reduce repeating titles, you could add a new parameter to this function for a dynamic title. Then you could simply call:
logsVerify({ params, title: 'Verify Logs' })
This way, the title can be customized without hardcoding it in multiple places.
| const installSteps = result.install(mockParams); | ||
| expect(installSteps).toHaveLength(1); | ||
| expect(installSteps[0].type).toBe('install'); | ||
| expect(installSteps[0].content).toHaveLength(2); | ||
|
|
||
| // Test configure step | ||
| const configureSteps = result.configure(mockParams); | ||
| expect(configureSteps).toHaveLength(1); | ||
| expect(configureSteps[0].type).toBe('configure'); | ||
| expect(configureSteps[0].content[1].code).toContain('options.EnableLogs = true;'); | ||
| expect(configureSteps[0].content[1].code).toContain(mockParams.dsn.public); | ||
|
|
||
| // Test verify step | ||
| const verifySteps = result.verify({isLogsSelected: true}); | ||
| expect(verifySteps).toHaveLength(1); | ||
| expect(verifySteps[0].type).toBe('verify'); | ||
| expect(verifySteps[0].content).toHaveLength(1); | ||
| expect(verifySteps[0].content[0].type).toBe('conditional'); | ||
| const conditionalContent = verifySteps[0].content[0].content; | ||
| expect(conditionalContent[1].code).toContain('SentrySdk.Logger.LogInfo'); |
There was a problem hiding this comment.
How about we test only the visual aspect? That way, if the structure changes, we won’t need to update this test. For example, you could just check for:
screen.getByText('SentrySdk.Logger.LogInfo')
There was a problem hiding this comment.
yeah makes sense. This was based on the existing logs.spec.tsx for Python, so I can adapt both.
follow-up question: Should we have this kind of test for each of the SDKs' logs pages?
| params, | ||
| 'sentry.dotnet.profiling', | ||
| '4.3.0' | ||
| '6.0.0' |
There was a problem hiding this comment.
should we have these changes in a separate PR? it seems unrelated to logs
Continuing from #102398, to be merged after that one was merged.
We add the
logsonboarding for each of the platforms that now support logs (unity, unreal, dotnet, native, php-symfony)ToDo
logscheckbox to project onboarding page for each of these SDKs (see original comment)