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

Refactored and added tests to Settings #28

Merged
merged 2 commits into from
Jun 5, 2024
Merged

Conversation

JonnyDeates
Copy link
Contributor

Adds tests for settings.

I haven't been able to find a way to mockout the Obsidian package. I spent awhile trying to get it bundled in, but I realized the only file provided in their Obsidian repo is a type file, not the actual module. So that was not the solution, then I tried creating a mock of the Obsidian repo, I made progress, but it was alot for little output, so I decided that a refactor for the settings and tests with that refactor could be helpful. I still think more test coverage is needed, and I am happy adding them if there is a solid way of getting around the Obsidian package throwing a fit.

I did also check some other repositories, it seems like most places just don't test any of the files that import from Obsidian.
I was looking off this list https://publish.obsidian.md/hub/04+-+Guides%2C+Workflows%2C+%26+Courses/Community+Talks/Plugin+Testing+for+Developers

EDIT: I just closed it, I hadn't rebased off of the master branch in a bit of a while, so this branch should be ready to merge.

@JonnyDeates
Copy link
Contributor Author

@benr77 I can try to add more tests, the main.ts, would need to be refactored some way some how, and the gitlab-api could have the Obsidian mock file, it does work, its just a headache to include.

I do TDD for work so I really don't mind adding tests, the annoyance of the obsidian node-module though is something I haven't had to deal with.

@JonnyDeates
Copy link
Contributor Author

I added some more tests. gitlab-api, gitlab-loader, default-template,

I also added a mocks file for Obsidian to get this working. The untested ones are now

filesystem.ts
main.ts
settings-tab.ts

@benr77
Copy link
Owner

benr77 commented Jun 4, 2024

This all looks good thanks for your efforts. Do you want me to merge it now or do you want to add more tests first?

I have very little free time available at the moment, and there does seem to be some interest in this project, so building up a decent test suite is a priority to allow merging contributions with confidence.

@JonnyDeates
Copy link
Contributor Author

I think it is good to merge in, those last files are a real pain to test, they each require a bunch of obsidian to be mocked, and sub functionality mocked. I could go through, but maybe sometime in the future. I think this branch is ready for what it is. Still those three files remain untested. Hopefully Obsidian creates a testing-module that does a lot of this headache for developers.

@benr77 benr77 merged commit 5dec7e5 into benr77:master Jun 5, 2024
1 check passed
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