-
-
Notifications
You must be signed in to change notification settings - Fork 200
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Unit tests #535
Unit tests #535
Conversation
Add npm script for jest with coverage for easier testing and add some documentation to helpers.ts
Add and improve unit tests for the Store class
Add tests for settings and index, configure GitHub actions for code coverage and automatic testing
Also update actions to newer versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most everything looks good. I had a few comments. Once those are done ill get it merged in.
// create a new store with 2 options | ||
describe('render module', () => { | ||
let render: Render | ||
let openMock: jest.Mock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really shouldnt need to mock anything. I really want to keep us actually testing things and for simple things like callback functions if we need to test whether it was triggered or not can be done without mocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's definitely true. I choose to use mocks because it simplified the test writing, because I tried to cover as much code as possible fairly quickly, I believed it to be a acceptable tradeoff. Writing the tests without mocks would complicate them quite a bit I think and I don't know if I've time for that.
Maybe it's OK to add them like this and rewrite when possible?
@@ -107,6 +107,19 @@ export default class Select { | |||
if (m.attributeName === 'class') { | |||
classChanged = true | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you describe what you are doing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new option was added that is now the selected value, therefore we trigger a change
event, which does not fire normally without user interaction. This fixes one of the old tests, which failed because the event was not fired before. I'm pretty sure that this is the way you expect it to work, but let me know
Revert comment changes and restore deleted public methods
Just let me know when your done and ill get it merged in |
The code coverage action keeps failing because it doesn't have write access to the PR, but I can't figure out a way of doing the coverage report otherwise. Do you have an idea how this can be improved? Otherwise I would consider the PR done. Improvements to the tests are something I'll look into in the future, there's still a lot to do. |
I personally dont care about code coverage in this instance so if its just easier to have it run tests for now I would be ok with that. |
Removed the action for now. Let's merge it 😄 |
This PR adds unit tests for a lot of code.
It also adds two GitHub Actions: one for jest that simply run the tests on pushes that change code and one for code coverage that runs on PRs.
The code coverage currently sits at 77% of statements (and LOCs) and I've set the threshold for the coverage check at 75% (maybe that's to high?).