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

Test coverage config adjustment #975

Closed
wants to merge 17 commits into from
Closed

Conversation

4Source
Copy link
Contributor

@4Source 4Source commented May 26, 2024

I have checked the config for jest and I have seen that the coverage is not really cover the full code so I adjusted the settings a bit to only include files there aren't testable because of include of obsidian api. Now the coverage looks different and we could improve on it.

@ronzulu
Copy link
Collaborator

ronzulu commented May 27, 2024

Hi @4Source you've been really powering through the issues lately, great work!

I haven't been very vocal lately, but have been busy on [FEAT] Refactor code to support diff methods of storing the scheduling info, and diff SR algorithms #878

FYI that has involved separating out more code to be unit testable.

Cheers
Ronny

@4Source
Copy link
Contributor Author

4Source commented May 27, 2024

Hi @4Source you've been really powering through the issues lately, great work!

Thanks, thought less open bug would result in more time for the great ideas there are already here and waiting for implementation.

I haven't been very vocal lately, but have been busy on [FEAT] Refactor code to support diff methods of storing the scheduling info, and diff SR algorithms #878

Great, then we can adjust the ignored files. What do you think about the global coverage threshold of 90% for now?

@4Source
Copy link
Contributor Author

4Source commented May 27, 2024

@ronzulu Not sure it was your choice but found code that is edited by you. Why the use of the moments lib and not the Date from build in javascript?

Because it seems the Moment lib has some issues with leap year.

@ronzulu
Copy link
Collaborator

ronzulu commented May 27, 2024

@ronzulu Not sure it was your choice but found code that is edited by you. Why the use of the moments lib and not the Date from build in javascript?

Because it seems the Moment lib has some issues with leap year.

I just checked and the plugin version when I started being involved was 1.10.1. It already used the Moment lib, from package.json:

"moment": "^2.29.4",

From memory I tried to standardize using it (instead of a mixture including raw Unix style times), but I didn't introduce it's usage.

Hope that helps!

Cheers
Ronny

@4Source
Copy link
Contributor Author

4Source commented May 27, 2024

From memory I tried to standardize using it (instead of a mixture including raw Unix style times), but I didn't introduce it's usage.

Ok then I will go through the code and check it could be replaced with the default Date I think this should have no issues with leap years. But I will test this first.

@ronzulu
Copy link
Collaborator

ronzulu commented Jun 15, 2024

Hi @4Source

Great, then we can adjust the ignored files. What do you think about the global coverage threshold of 90% for now?

If a file is currently in the coverage list and someone modifies it without updating the unit tests, the coverage test will fail, which I think is good.

If we change the global coverage threshold, I assume this means we lose that future protection. Is that how you see it?

@4Source
Copy link
Contributor Author

4Source commented Jun 15, 2024

If a file is currently in the coverage list and someone modifies it without updating the unit tests, the coverage test will fail, which I think is good.

Yes this is indeed good I meant to reduce the amount of files there are currently on ignore because of obsidian api dependency and the threshold of 90% because of lot of test need to be written to increase the coverrage but I think the target should be to test over 95% of the code

@4Source
Copy link
Contributor Author

4Source commented Jun 15, 2024

I will add more tests to this branch to increase coverage when I have the time.

@ronzulu
Copy link
Collaborator

ronzulu commented Jun 16, 2024

Yes this is indeed good I meant to reduce the amount of files there are currently on ignore because of obsidian api dependency and the threshold of 90% because of lot of test need to be written to increase the coverrage but I think the target should be to test over 95% of the code

As part of #878 I've had to separate out more code to be independent of the Obsidian api and added more test cases. I wish I had more time to complete this...

Do you know if jest can be configured for 100% coverage of specific files, and 95% across all the code?

@4Source
Copy link
Contributor Author

4Source commented Jun 16, 2024

Do you know if jest can be configured for 100% coverage of specific files, and 95% across all the code?

Yes this is possible. You can configure single files or folders

@st3v3nmw
Copy link
Owner

Hi @4Source, I rebased #1056 against your branch so almost all of these changes are in master now. We can probably close this now.. Thanks!

# Conflicts:
#	jest.config.js
#	src/lang/locale/sw.ts
#	src/utils/utils.ts
#	tests/unit/sample-items.ts
#	tests/unit/utils/NumberCountDict.test.ts
#	tests/unit/utils/utils.test.ts
@4Source
Copy link
Contributor Author

4Source commented Sep 21, 2024

  1. Change only the case on windows and get git recognize it sucks
  2. Why is the test coverage on my device different than in the action
  3. Why is the test coverage changing without changing code

@st3v3nmw
Copy link
Owner

st3v3nmw commented Sep 22, 2024

Change only the case on windows and get git recognize it sucks

Sorry I don't understand what this means. If it's related to the isEqualOrSubPath function, that function is no longer in use.

Why is the test coverage on my device different than in the action
Why is the test coverage changing without changing code

I'm not too sure either. I've ran into the same discrepancies (the coverage is higher locally) yet the node versions & dependencies are the same locally & on the GitHub action 🤔.

@4Source
Copy link
Contributor Author

4Source commented Sep 23, 2024

Sorry I don't understand what this means. If it's related to the isEqualOrSubPath function, that function is no longer in use.

No, sorry, I should have made that clearer. Since I created the branch, the files have been renamed and Git on Windows seems to have trouble recognizing naming changes when it's just the case

I'm not too sure either. I've ran into the same discrepancies (the coverage is higher locally) yet the node versions & dependencies are the same locally & on the GitHub action 🤔.

Interesting

# Conflicts:
#	jest.config.js
Copy link
Owner

@st3v3nmw st3v3nmw left a comment

Choose a reason for hiding this comment

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

We'll have to close this @4Source. I added almost all of these PR's changes through #1056. What's remaining in the diff only updates the jest config. The package-lock.json is unnecessary since we use pnpm & its pnpm-lock.yaml.

@st3v3nmw st3v3nmw closed this Sep 25, 2024
@4Source 4Source deleted the test_coverage branch September 25, 2024 09:27
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.

3 participants