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

Add test save #686

Merged
merged 8 commits into from
Dec 1, 2023
Merged

Add test save #686

merged 8 commits into from
Dec 1, 2023

Conversation

francescalb
Copy link
Collaborator

@francescalb francescalb commented Nov 30, 2023

Description

Type of change

  • Bug fix.
  • New feature.
  • Documentation update.
  • Test update.

Checklist

This checklist can be used as a help for the reviewer.

  • Is the code easy to read and understand?
  • Are comments for humans to read, not computers to disregard?
  • Does a new feature has an accompanying new test (in the CI or unit testing schemes)?
  • Has the documentation been updated as necessary?
  • Does this close the issue?
  • Is the change limited to the issue?
  • Are errors handled for all outcomes?
  • Does the new feature provide new restrictions on dependencies, and if so is this documented?

Comments

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7fd9eae) 70.64% compared to head (80e4673) 71.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #686      +/-   ##
==========================================
+ Coverage   70.64%   71.00%   +0.35%     
==========================================
  Files          16       16              
  Lines        3369     3369              
==========================================
+ Hits         2380     2392      +12     
+ Misses        989      977      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@francescalb francescalb marked this pull request as ready for review November 30, 2023 15:54
from pathlib import Path

# Save ontology in a different location
testonto.save(tmpdir / "testonto_saved.ttl")
Copy link
Collaborator

@jesper-friis jesper-friis Nov 30, 2023

Choose a reason for hiding this comment

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

I stedet for å skrive til en temporær mappe som automatisk slettes når testen har kjørt, er det enklere å debugge hvis man skriver til en output/ undermappe under tests/. Hvis du legger inn en .gitignore fil i den mappen som inneholder en linje med "*", vill den mappen bli en del av git-repoet men alle filene som blir generert i den vill bli ignorert av git. Men de vill fortsatt være enkelt tilgjengelige for debugging.

Eneste ulempen med en slik løsning er at output/ ikke er garantert å være tom ved start av testen. Dvs. man må sette overwrite=True. At overwrite fungerer må selvfølgelig testes eksplisitt som du gjør under.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer to make use of temporary directories that are cleaned up after use and that tests are run in a clean directory every time if that is the intended purpose. Overwrite=False is the default, I don't want to have overwrite=True unless I want to specifically test that. I have added a suggestion for writing to a local directory in debug mode (setting debug=True). Then we meet half way :)

Copy link
Collaborator

@jesper-friis jesper-friis Nov 30, 2023

Choose a reason for hiding this comment

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

As you wish... but I really don't see the point of such complexity. Why introducing different test modes? The tests are not part of the distributed production code, but ment to be simple and helpful for ensuring code quality. Easy debugging is part of that.

All extra complexity added to the tests just make the code harder to maintain - and will sooner or later require that we have to add tests for the tests...

tests/test_save.py Outdated Show resolved Hide resolved
Comment on lines +64 to +70
testonto.save(format="rdfxml", dir=tmpdir)
# 3.
with open(tmpdir / "testonto_saved.owl") as f:
owlfile = f.read()
with open(tmpdir / "testonto.rdfxml") as f:
owlfile2 = f.read()
# assert owlfile != owlfile2 # to be uncommented when issue #685 is fixed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hvorfor skal owlfile og owlfile2 være ulike. Har du endret på testonto?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because I have saved testonto.rdfxml again. Since overwrite=False is the default it should append to it, thus make the two files different. Stated it even more explicitly in the comment aboe (step2)

Copy link
Collaborator

@jesper-friis jesper-friis Nov 30, 2023

Choose a reason for hiding this comment

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

The only thing overwrite=True does is to remove the destination before writing to it. Without overwrite, the behaviour is defined by owlready2 for rdfxml and rdflib for turtle. I haven't checked their documentation, but I don't expect that any of them will duplicate a triple when writing twice to the file.

@francescalb francescalb merged commit 41cafbb into master Dec 1, 2023
12 checks passed
@francescalb francescalb deleted the add_test_save branch December 1, 2023 09:13
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.

2 participants