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 testing support #88

Closed
wants to merge 10 commits into from
Closed

Conversation

Hidorikun
Copy link
Contributor

Added a way to run all tests.
Added tests for the remove punctuation utility as well as refactored it.
Moved constants in separate folder because they caused a circular dependency between ts_core and library.

Copy link
Collaborator

@Loran425 Loran425 left a comment

Choose a reason for hiding this comment

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

Glad to see someone interested in the testing side of thing, many new projects (or newly open source projects) suffer due to lack of testing, and having robust tests makes everyone's lives easier.

A few other changes that would be needed before this could be considered for a merge.
The following files all reference ts_core.py looking for items now in the src.core.constants file.

tagstudio\src\qt\modals\build_tag.py
tagstudio\src\qt\widgets\collage_icon.py
tagstudio\src\qt\widgets\item_thumb.py
tagstudio\src\qt\widgets\preview_panel.py
tagstudio\src\qt\widgets\thumb_renderer.py

Comment on lines 19 to 22
from tagstudio.src.core.json_typing import JsonCollation, JsonEntry, JsonLibary, JsonTag
from tagstudio.src.core.constants import TS_FOLDER_NAME, BACKUP_FOLDER_NAME, COLLAGE_FOLDER_NAME, VERSION, TEXT_FIELDS
from tagstudio.src.core.utils.str import strip_punctuation
from tagstudio.src.core.utils.web import strip_web_protocol
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tagstudio.src... import format breaks at runtime for vscode and when running from the terminal

Suggested change
from tagstudio.src.core.json_typing import JsonCollation, JsonEntry, JsonLibary, JsonTag
from tagstudio.src.core.constants import TS_FOLDER_NAME, BACKUP_FOLDER_NAME, COLLAGE_FOLDER_NAME, VERSION, TEXT_FIELDS
from tagstudio.src.core.utils.str import strip_punctuation
from tagstudio.src.core.utils.web import strip_web_protocol
from src.core.json_typing import JsonCollation, JsonEntry, JsonLibary, JsonTag
from src.core.constants import TS_FOLDER_NAME, BACKUP_FOLDER_NAME, COLLAGE_FOLDER_NAME, VERSION, TEXT_FIELDS
from src.core.utils.str import strip_punctuation
from src.core.utils.web import strip_web_protocol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback! I have made the changes.

Comment on lines 10 to 11
from tagstudio.src.core.constants import TEXT_FIELDS
from tagstudio.src.core.library import Entry, Library
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tagstudio.src... import format breaks at runtime for vscode and when running from the terminal

Suggested change
from tagstudio.src.core.constants import TEXT_FIELDS
from tagstudio.src.core.library import Entry, Library
from src.core.constants import TEXT_FIELDS
from src.core.library import Entry, Library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback! I have made the changes.

Comment on lines 47 to 49
from tagstudio.src.core.utils.web import strip_web_protocol
from tagstudio.src.qt.flowlayout import FlowLayout, FlowWidget
from tagstudio.src.qt.main_window import Ui_MainWindow
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tagstudio.src... import format breaks at runtime for vscode and when running from the terminal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback! I have made the changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm having luck getting python.exe -m unittest discover tagstudio.tests -t "tagstudio" to run from the <local_repo> directory but this file is throwing a fit working between all the different use cases.
Something like unittest.TestSuite() or unittest load test protocol might be more appropriate for the longer term and avoid needing to change the import format in the src library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added pytest instead of unittest and I found that running python -m pytest tests/ from the tagstudio folder works. Does not seem to work any other way though.

requirements.txt Outdated
@@ -7,3 +7,4 @@ PySide6_Addons>=6.5.1.1,<=6.6.3.1
PySide6_Essentials>=6.5.1.1,<=6.6.3.1
typing_extensions>=3.10.0.0,<=4.11.0
ujson>=5.8.0,<=5.9.0
parametrized==0.9.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
parametrized==0.9.0
parameterized==0.9.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback! I have added a requirements-dev.txt as discussed in this MR with pytest.

FixDupeFilesModal, FoldersToTagsModal)
import src.qt.resources_rc
import tagstudio.src.qt.resources_rc
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tagstudio.src... import format breaks at runtime for vscode and when running from the terminal

Suggested change
import tagstudio.src.qt.resources_rc
import src.qt.resources_rc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback! I have made the changes.


from tagstudio.src.core.library import Tag
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tagstudio.src... import format breaks at runtime for vscode and when running from the terminal

Suggested change
from tagstudio.src.core.library import Tag
from src.core.library import Tag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback! I have made the changes.


from parameterized import parameterized

from tagstudio.src.core.utils.str import strip_punctuation
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tagstudio.src... import format breaks at runtime for vscode and when running from the terminal

Suggested change
from tagstudio.src.core.utils.str import strip_punctuation
from src.core.utils.str import strip_punctuation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback! I have made the changes.

from src.core.library import ItemType
from src.core.ts_core import (PLAINTEXT_TYPES, TagStudioCore, TAG_COLORS, DATE_FIELDS, TEXT_FIELDS, BOX_FIELDS, ALL_FILE_TYPES,
from tagstudio.src.core.library import Collation, Entry, ItemType, Library, Tag
from tagstudio.src.core.palette import ColorType, get_tag_color
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line was removed in a previous commit and is no longer used in ts_qt.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback! I have made the changes.


from src.core.library import ItemType
from src.core.ts_core import (PLAINTEXT_TYPES, TagStudioCore, TAG_COLORS, DATE_FIELDS, TEXT_FIELDS, BOX_FIELDS, ALL_FILE_TYPES,
from tagstudio.src.core.library import Collation, Entry, ItemType, Library, Tag
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line was previously reduced to only import ItemType as it is the only item used in ts_qt.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback! I have made the changes.

requirements.txt Outdated
@@ -7,3 +7,4 @@ PySide6_Addons>=6.5.1.1,<=6.6.3.1
PySide6_Essentials>=6.5.1.1,<=6.6.3.1
typing_extensions>=3.10.0.0,<=4.11.0
ujson>=5.8.0,<=5.9.0
parametrized==0.9.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

this shouldnt be a production depedency, I'd suggest to create requirements-dev.txt or similar and put it there instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. By the way, do you guys manually add those dependencies to the requirements.txt or can pip do that automtically like npm can?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's pip freeze which lists all currently installed packages, so you can do pip freeze > requirements.txt but it lists even the transitional dependencies which is suboptimal.

Ideally the project should use some advanced dep. management tool like Poetry or pdm, but seems like that isnt/wasnt a priority yet

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair point,
I don't think there's currently a plan to switchover to a more robust dependency manager, not saying a switch shouldn't happen but I believe the energy is more focused on swapping the backend from a JSON file to a database.

Are there any recommendations aside from something like a requirements-dev.txt file to be a stopgap on this kind of thing?

Example

-r requirements.txt
parameterized==0.9.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd go with requirements-dev.txt and requirements-dev.txt without referencing each other, any dev will know how to deal with it. But I dont have strong opinion about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you guys think about the latest changes? 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd keep the original name requirements.txt instead of adding the -prod suffix there, as that's the most common name everyone looks for by default. Other than that it looks good to me, nice job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thank you for the review 😄

@yedpodtrzitko
Copy link
Collaborator

just out of curiosity - why unittest instead of pytest? The latter is considered defacto the go-to testing library. It has fixtures and parametrization support out of the box, and it's less verbose, eg. no need to wrap every single test into a class, and you can use plain assert instead of writing self.assertEqual etc. everywhere.

@Hidorikun
Copy link
Contributor Author

@yedpodtrzitko I was looking for some native support for testing in Python and came across unittest.

My thinking was that I would want to avoid adding extra dependencies if I can.
Saying that, to create a parameterized test I also needed to add a dependency, so if pytest can do that and much more, I agree that it would be a better alternative.

Thank you for pointing it out. I am not accustomed with the defacto conventions in python projects so far, and I appreciate the input.

Copy link
Collaborator

@Loran425 Loran425 left a comment

Choose a reason for hiding this comment

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

Great work, I'm only seeing these two comments that should be addressed.

tagstudio/src/core/ts_core.py Outdated Show resolved Hide resolved
@@ -323,7 +323,7 @@ def start(self):
# self.entry_panel.setWindowIcon(icon)

if os.name == 'nt':
appid = "cyanvoxel.tagstudio.9"
appid = "cyanvoxel.9"
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure if this was intentional or a replace all gone wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a replace gone wrong, good catch. I reverted it.

Hidorikun and others added 2 commits May 1, 2024 11:11
Co-authored-by: Andrew Arneson <Loran425@gmail.com>
Comment on lines 1 to 9
humanfriendly==10.0
opencv_python>=4.8.0.74,<=4.9.0.80
Pillow==10.3.0
pillow_avif_plugin>=1.3.1,<=1.4.3
PySide6>=6.5.1.1,<=6.6.3.1
PySide6_Addons>=6.5.1.1,<=6.6.3.1
PySide6_Essentials>=6.5.1.1,<=6.6.3.1
typing_extensions>=3.10.0.0,<=4.11.0
ujson>=5.8.0,<=5.9.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
humanfriendly==10.0
opencv_python>=4.8.0.74,<=4.9.0.80
Pillow==10.3.0
pillow_avif_plugin>=1.3.1,<=1.4.3
PySide6>=6.5.1.1,<=6.6.3.1
PySide6_Addons>=6.5.1.1,<=6.6.3.1
PySide6_Essentials>=6.5.1.1,<=6.6.3.1
typing_extensions>=3.10.0.0,<=4.11.0
ujson>=5.8.0,<=5.9.0

There was probably some misunderstanding, but these dependencies should remain only in requirements.txt, otherwise these files will get out of sync sooner or later and it will cause problems.

If you really want to have everything installed via a single dependency file, put here -r requirements.txt isntead. Otherwise I think both these files should be independent from each other. Ie. requirements.txt for installing production dependencies, requirements-dev.txt for developers' dependencies.

@gabrieljreed
Copy link
Contributor

Just wanted to add my 2 cents here - this looks great! One small thing I might change is that I usually see the tests folder live outside the source code directory (in this case, the tagstudio folder.) Everything inside the source code folder should be code for the package, since tests are not actual application code, they can be separate. This is actually recommended in the pytest documentation as well.

@Hidorikun Hidorikun mentioned this pull request May 6, 2024
@Hidorikun Hidorikun closed this May 7, 2024
@Hidorikun Hidorikun deleted the test-support branch May 7, 2024 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants