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.

Sign up for GitHub

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

Jump to bottom

Add tests to credentialManager #95

Merged
merged 5 commits into from
Jul 7, 2021
Merged

Add tests to credentialManager #95

merged 5 commits into from
Jul 7, 2021

Conversation

YouKnowBlom
Copy link
Contributor

Add constructor to credentialManager with optional parameter to initialize the store with.
Add tests for methods in credentialManager.
Remove --passWithNoTests from test command to disallow passing when no tests are found.

@dkanada
Copy link
Member

dkanada commented Jul 3, 2021

@YouKnowBlom this one also seems good to merge if you rebase against master and optionally address the comments.

@YouKnowBlom YouKnowBlom force-pushed the test-credman branch 2 times, most recently from 93e9606 to 87a7a81 Compare July 3, 2021 20:58
@YouKnowBlom YouKnowBlom marked this pull request as draft July 3, 2021 21:45
@dkanada
Copy link
Member

dkanada commented Jul 4, 2021

Is this still failing after you rebased against master? I'm less keen on merging it if the new tests will break the CI workflows.

@YouKnowBlom
Copy link
Contributor Author

Yeah it is broken right now. The fix is setting some values in the tsconfig but I haven't gotten around to it yet, I'll let you know when it's done

@YouKnowBlom
Copy link
Contributor Author

Now it should be working! I fixed the linting issues in the related files but there are many unrelated warnings which will have to go into a seperate PR.

@YouKnowBlom YouKnowBlom marked this pull request as ready for review July 4, 2021 20:39
@dkanada
Copy link
Member

dkanada commented Jul 4, 2021 •
edited
Loading

Why is the lint failing now out of nowhere? There aren't really any changes to enable new checks here. If nothing else, I would rather disable the checks that are throwing errors until they're actually fixed in the codebase to avoid collecting new issues.

@YouKnowBlom
Copy link
Contributor Author

I'm confused as well. I'll take a closer look tonight to see if I can figure it out. If not I'll disable them for now

@YouKnowBlom
Copy link
Contributor Author

Apparently the prettier was mad that the formatting changed in tsconfig and was returning 1, fixed now! @dkanada

@dkanada dkanada merged commit 28a75ba into jellyfin:master Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkanada dkanada dkanada approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@YouKnowBlom @dkanada