[Internals] Improve unit tests #4262
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Question: can/should we merge the Lint + Unit CI tasks into one, so that the dependencies only need to be installed once? |
ghost
suggested changes
Jan 21, 2020
| @@ -10,7 +10,7 @@ describe('get_name_from_filename', () => { | |||
| assert.equal(get_name_from_filename('path/to/Widget/index.svelte'), 'Widget'); | |||
| }); | |||
|
|
|||
| it('handles unusual filenames', () => { | |||
There was a problem hiding this comment.
I don't think you want to replace these lines but add a case for Windows instead.
There was a problem hiding this comment.
https://github.com/sveltejs/svelte/pull/4262/files#diff-7c79613acd5ee2529bb07bd48bb98cd4R14
☝️ I added a test case which covers the original intent of the test.
jesseskinner
pushed a commit
to jesseskinner/svelte
that referenced
this pull request
Feb 27, 2020
taylorzane
pushed a commit
to taylorzane/svelte
that referenced
this pull request
Dec 17, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While implementing #3311 (and adding some unit tests for some new util functions I wrote), I noticed that the existing unit tests:
This ticket fixes the first item by making the test more valuable and testing the real case that it was meant to catch (for Windows filenames -- see #3862).
It addresses the second issue by adding unit testing to the CI tasks. Not sure if this is actually something we want seeing as we're only testing one module (for now).
I hope that this encourages more unit testing in the future (although I think the integration/other tests are probably solid enough already). In general I think unit tests are pretty cheap, but one thing they are good for (especially in a large OS project) is for documenting how code works, making it easier for folks new to the project to understand. 👍
Resolves #4261