(BSR) perf(firebase): gestion des performances par firebase #7271
Conversation
pcanthelou-pass
changed the title
pcanthelou-pass
force-pushed
the
bsr/firebaseperfv1
branch
from
be0e598
to
8d18136
Compare
pcanthelou-pass
force-pushed
the
bsr/firebaseperfv1
branch
from
8d18136
to
a09b71a
Compare
|
|
There was a problem hiding this comment.
Hello, je passe juste par là, stylé que tu ais les mains encore dans le code, je te fait des retours sur les bonnes pratiques au sein de l'équipe plus que sur la partie métier.
- essaie de remplacer tous les fichiers
.tsxqui n'utilisent pas dejsxpar des fichiersts - remplace les
test('...')parit('should ...')
|
|
||
| jest.mock('shared/performance/useFirebasePerformanceProfiler', () => ({ | ||
| useFirebasePerformanceProfiler: jest.fn(), | ||
| })) |
There was a problem hiding this comment.
Il faudrait mettre ce mock dans les fichiers de tests concernés, dans le cas contraire ce mock sera chargé même dans les fichiers qui ne l'utilisent pas #6531
| const { isConnected } = useNetInfoContext() | ||
| const { isLoggedIn } = useAuthContext() | ||
|
|
||
| useFirebasePerformanceProfiler('Favorites', { route } as UsePerformanceProfilerOptions) |
There was a problem hiding this comment.
le as peut-être supprimé
| @@ -158,6 +152,8 @@ export const ThematicHome: FunctionComponent = () => { | |||
| // eslint-disable-next-line react-hooks/exhaustive-deps | |||
| }, [hasGeolocPosition, isFromDeeplink]) | |||
|
|
|||
| useFirebasePerformanceProfiler('ThematicHome', { route } as UsePerformanceProfilerOptions) | |||
There was a problem hiding this comment.
on peut supprimer le as
| class MyDate extends Date { | ||
| constructor(date: string) { | ||
| super(date) | ||
| } | ||
| now = () => 1732384186972 | ||
| } |
There was a problem hiding this comment.
Bien vu pour l'injection de dépendance, mais on ne l'utilise pas dans le code pour le moment on utilise mockdate.set(date)
On va préférer avoir un objet avec des fonctions comme la majorité de l'équipe n'a pas de connaissance sur l'OOP
There was a problem hiding this comment.
on peut peut-être utiliser un autre nom de classe que le MyDate
|
|
||
| describe('AppStartTimeStore', () => { | ||
| test('When start time the start time is correct', () => { | ||
| const t = MyDate.now() |
There was a problem hiding this comment.
on peut peut-être utiliser un autre nom de classe que le t
| // Supprimer les espaces en début et fin | ||
| let sanitized = name.trim() | ||
| // Supprimer les underscores au début et à la fin | ||
| sanitized = sanitized.replace(/(^_+)|(_+$)/g, '') | ||
| // Tronquer à 32 caractères maximum |
There was a problem hiding this comment.
les commentaires peuvent peut-être supprimés pour un naming plus parlant ou un stringBuilder pour encapsuler les implémentation de méthodes et exposer des noms parlant (overkill)
| @@ -0,0 +1,324 @@ | |||
| /* eslint-disable no-restricted-imports */ | |||
There was a problem hiding this comment.
import { act, renderHook } from 'tests/utils' et renommer en .ts
| import { PerformanceService, InteractionService } from './types' | ||
| import { usePerformanceProfiler } from './usePerformanceProfiler' | ||
|
|
||
| test('Ne fait rien si on ne veut pas profiler', async () => { |
There was a problem hiding this comment.
Peut-être mettre des noms de tests en anglais et wrapper le tout avec un describe
| type useFirebasePerformanceProfilerProps = { | ||
| route?: Route | ||
| shouldProfile?: boolean | ||
| } | ||
|
|
||
| export const useFirebasePerformanceProfiler = ( | ||
| traceName: string, | ||
| props?: useFirebasePerformanceProfilerProps | ||
| ) => { |
There was a problem hiding this comment.
type useFirebasePerformanceProfilerProps<T extends ScreenNames> = {
route?: UseRouteType<T>
shouldProfile?: boolean
}
export const useFirebasePerformanceProfiler = <T extends ScreenNames>(
traceName: T,
props?: useFirebasePerformanceProfilerProps<T>
) => {ce type te permettra d'avoir l'autocomplétion avec toutes les routes et de supprimer le problème avec les as
There was a problem hiding this comment.
Et bien remplacer le nom du fichier en ts car le tsx à tendance à confondre <T extends ScreenNames> avec des composants
Link to JIRA ticket: https://passculture.atlassian.net/browse/PC-XXXXX
Flakiness
If I had to re-run tests in the CI due to flakiness, I add the incident on Notion
Checklist
I have:
Screenshots
delete if no UI change
Best Practices
Click to expand
!when you know that the value can’t benullorundefined).Test specific:
usertofireEvent.