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

Removing file artifacts generated from tests #160

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

simojoe
Copy link
Contributor

@simojoe simojoe commented May 4, 2023

La suite de test gérére présentement un fichier TestClassOutput.scala, qui a été commit à deux endroits différents dans le code.

Ce fichier est généré par ClassGenerator.writeCLassFile

(petite commentaire, doit-on dire write-C-Lass-File ou bien c'est un typo)

La fonction est présentement utilisée dans deux tests :

  • "class generator" should "create folder if not exists"
    • Teste l'écriture d'un fichier dans un dossier inexistant dans /target
  • "class generator" should "add import for Timestamps"
    • Teste la génération du contenu du fichier et écrit un fichier pas dans /target

Je crois qu'il est prudent de retirer le code la commit e4b195a puisque :

  • Le call est déjà couverte par un autre test
  • Le call n'est pas pertinent à la réussite du test

@simojoe simojoe requested a review from jecos May 4, 2023 21:31
@@ -138,12 +138,6 @@ case class TestClassOutput(`a`: Option[String] = None,
Seq(TestInput(d = Seq())).toDF,
LocalDateTime.of(1900, 1, 1, 0, 0, 0))

ClassGenerator.writeCLassFile(
Copy link
Member

Choose a reason for hiding this comment

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

Je suis d'accord que ce call ne sert à rien dans la réussite du test, mais le test "class generator" should "create folder if not exists" ne vérifie pas le résultat de l'écriture. On pourrait le retirer du test de Timestamp, mais écrire un test qui vérifie que le fichier écrit par writeCLassFile est valide?

Copy link
Member

Choose a reason for hiding this comment

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

Oui le test est pertinent en effet. Par contre on devrait deleter le fichier du repo et ajouter une entrée dans le .gitignore

@@ -138,12 +138,6 @@ case class TestClassOutput(`a`: Option[String] = None,
Seq(TestInput(d = Seq())).toDF,
LocalDateTime.of(1900, 1, 1, 0, 0, 0))

ClassGenerator.writeCLassFile(
Copy link
Member

Choose a reason for hiding this comment

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

Effectivement, c'est un typo. À remplacer par writeClassFile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants