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

[#154] Add UI-based unit tests for demo application #197

Merged
merged 88 commits into from
Nov 20, 2024

Conversation

Tayebsed93
Copy link
Collaborator

@Tayebsed93 Tayebsed93 commented Oct 16, 2024

Note: Please transform - [ ] into - (NA) in the description when things are not applicable

Related issues

#154

Description

Add unit tests for screenshot comparisons

Motivation & Context

Types of change

  • New feature (non-breaking change which adds functionality)

Previews

Checklist

Contribution

Accessibility

  • (NA) My change follows accessibility good practices

Design

  • (NA) My change respects the design guidelines of Orange Unified Design System

Development

  • My change follows the developer guide
  • I checked my changes do not add new SwiftLint warnings
  • I have added unit tests to cover my changes (optional)

Documentation

  • My change introduces changes to the documentation and/or I have updated the documentation accordingly

Checklist (for Core Team only)

  • (NA) The evolution have been tested and the project builds for iPhones and iPads
  • Code review has been done by reviewers according to CODEOWNERS file
  • (NA) Design review has been done
  • (NA) Accessibiltiy review has been done
  • (NA) Q/A team has tested the evolution
  • Documentation has been updated if relevant
  • Internal files have been updated if relevant (like CONTRIBUTING, DEVELOP, THIRD_PARTY, CONTRIBUTORS, NOTICE)
  • CHANGELOG has been updated respecting keep a changelog rules and reference the issues

@Tayebsed93 Tayebsed93 linked an issue Oct 16, 2024 that may be closed by this pull request
15 tasks
@Tayebsed93 Tayebsed93 force-pushed the 154-demo-app-add-more-ui-tests-on-demo-app branch from 949f27a to da6e278 Compare October 17, 2024 08:11
@pylapp pylapp changed the title 154 demo app add more UI tests on demo app [#154] Add UI-based uni tests for demo application Oct 17, 2024
@pylapp pylapp force-pushed the 154-demo-app-add-more-ui-tests-on-demo-app branch from da6e278 to ab3ce0a Compare October 17, 2024 13:03
@pylapp pylapp marked this pull request as draft October 17, 2024 13:28
@pylapp

This comment was marked as outdated.

@pylapp pylapp changed the title [#154] Add UI-based uni tests for demo application [#154] Add UI-based unit tests for demo application Oct 17, 2024
@Tayebsed93 Tayebsed93 force-pushed the 154-demo-app-add-more-ui-tests-on-demo-app branch 2 times, most recently from ac9c17e to bd55ec1 Compare October 17, 2024 15:43
@Tayebsed93
Copy link
Collaborator Author

@pylapp Merci pour ton retour. Désormais, j'arrive à voir les warnings et plus de 250 d'entre eux sont corrigés. Les fonctions de test ont aussi été modifiées. Le rebase est fait

Copy link
Member

@pylapp pylapp left a comment

Choose a reason for hiding this comment

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

La PR est loin d'être acceptable en l'état.

Il ne faut pas que tu corriges bêtement les warnings sans faire attention à ce que tu fais, surtout quand tu apportes de l'incohérence entre les fichiers du projet.

Regarde des classes de tests à côté ; tu pourras grandement t'en inspirer.

Dans les points à corriger :

  1. Il faut des sauts de lignes entre les fonctions, c'est la base ;
  2. le warning que tu as voulu corriger ne te dit pas de supprimer la ligne, mais les espaces inutiles en début de ligne ;
  3. Pour les classes de tests on n'a pas besoin de deinit, pour le coup tu peux désactiver proprement ce warning comme indiqué en commentaire ;
  4. Tu as réussi à mettre des fonctions de tests en dehors de leur classe de test ;
  5. On ajoute les nouvelles lignes du CHANGELOG par le haut ; ta ligne est totalement perdue dans une section ;
  6. Aucune documentation, tant pour les classes de tests que dans le DEVELOP par exemple ;
  7. Les MARKS sont utilisés n'importe comment ;
  8. Pourquoi on ne teste parfois que pour le color scheme light et pas le dark aussi ?
  9. Tu rajoutes du code qui ne semble n'avoir aucun rapport avec ce que tu dois faire dans ta PR.

Un MARK est une balise qui sert d'ancrage dans la minimap de Xcode, je te laisse chercher. Si tu ajoutes après les : un tiret, une ligne horizontale apparait pour séparer visuellement les choses. L'intérêt des MARK est d'apporter une aide visuelle dans le code, avec une structure claire. Deux MARK qui se suivent ne servent à rien.

Par exemple, si je prends :

    // MARK: - Tests
    // MARK: - Orange Theme Light Mode Width Tests
    // MARK: - Width Tokens Tests

Le seul MARK utile est ici :

    // MARK: - Orange Theme Light Mode Width Tests

D'ailleurs, je ne comprends pas comment pour un même code modulo la valeur du paramètre "named" tu peux définir des cas de tests différents.
Il faudrait que tu précises à la fin de la rubrique "UI tests in demo app" de DEVELOP comment ça fonctionne. Si je ne comprends pas, d'autres ne comprendront pas. Si on doit te demander comment ça fonctionne, c'est qu'il y a un problème a minima de documentation quelque part. Si tu n'es pas à l'aise en anglais pour expliquer clairement comment ça fonctionne, rédige le paragraphe en français en commentaire de cette PR et je traduirais.

Au besoin, demande de l'aide à @ludovic35 qui avait travaillé sur le sujet.

@Tayebsed93 Tayebsed93 force-pushed the 154-demo-app-add-more-ui-tests-on-demo-app branch 4 times, most recently from 29af724 to 4ec9673 Compare October 24, 2024 07:21
@pylapp pylapp force-pushed the develop branch 3 times, most recently from 547eb36 to d68fddd Compare October 24, 2024 08:29
@Tayebsed93 Tayebsed93 force-pushed the 154-demo-app-add-more-ui-tests-on-demo-app branch 4 times, most recently from 76eba6a to 0624115 Compare October 29, 2024 15:24
@Tayebsed93 Tayebsed93 assigned pylapp and Tayebsed93 and unassigned Tayebsed93 and pylapp Oct 31, 2024
@Tayebsed93 Tayebsed93 marked this pull request as ready for review October 31, 2024 10:37
@pylapp pylapp force-pushed the 154-demo-app-add-more-ui-tests-on-demo-app branch from 3f8c1f9 to 96d7bf5 Compare November 20, 2024 10:11
@pylapp pylapp merged commit b0d8224 into develop Nov 20, 2024
6 of 7 checks passed
@pylapp pylapp deleted the 154-demo-app-add-more-ui-tests-on-demo-app branch November 20, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Demo App] Add more UI tests on demo app
3 participants