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 for is(value) #86

Merged
merged 5 commits into from
Apr 28, 2019
Merged

Test coverage for is(value) #86

merged 5 commits into from
Apr 28, 2019

Conversation

MrHen
Copy link
Contributor

@MrHen MrHen commented Apr 19, 2019

Fixes #7.

I chose a slightly different path than the one suggested in the issue. There were only two minor annoyances that you might want cleaned up / tweaked:

  • For some reason, TypeName.GeneratorFunction has a value of 'GeneratorFunction' but the value returned from is is 'Generator'. If the actual string of 'GeneratorFunction' is not being used, could we change it to 'Generator'?
  • The various HTML elements all have different types that are not included in TypeName. They were also mixed into a single test type of domElement which meant that the typename helper would not work properly. I don't think anything needs to be changed here; just calling it out.

@MrHen MrHen changed the title Feature/7 test is Test coverage for is(value) Apr 19, 2019
@MrHen MrHen changed the title Test coverage for is(value) Test coverage for is(value) Apr 19, 2019
Copy link
Collaborator

@gioragutt gioragutt left a comment

Choose a reason for hiding this comment

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

Regarding TypeName.GeneratorFunction, if GeneratorFunction is not the actual technical term, and it really is Generator, we should rename it to Generator.

Other than that, looks like a solid PR. GJ man.

test/test.ts Outdated
@@ -753,6 +800,13 @@ test('is.inRange', t => {
test('is.domElement', t => {
testType(t, 'domElement');
t.false(is.domElement({nodeType: 1, nodeName: 'div'}));

t.is(is(createDomElement('div')), 'HTMLDivElement');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd refactor this to something like:

const htmlTagNameToTypeName = {
    span: 'HTMLSpanElement',
    ...
}

Object.entries(htmlTagNameToTypeName).forEach(([tagName, typeName]: string[]) => {
    const domElement = createDomElement(tagName);
    t.is(is(domElement), typeName);
})

test/test.ts Show resolved Hide resolved
@MrHen
Copy link
Contributor Author

MrHen commented Apr 19, 2019

Turns out GeneratorFunction does need to be 'GeneratorFunction' so I just added Generator as its own entry in the enum.

@sindresorhus sindresorhus merged commit 373605e into sindresorhus:master Apr 28, 2019
@sindresorhus
Copy link
Owner

🙌

@MrHen MrHen deleted the feature/7-Test-Is branch April 28, 2019 13:11
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.

is(value) is not slightly tested
3 participants