-
Notifications
You must be signed in to change notification settings - Fork 1
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
Incremental UI tests #2 #81
Conversation
client/src/components/utils.test.tsx
Outdated
}); | ||
|
||
describe('shortenAuthors', () => { | ||
// TODO EM: would this ever happen? Can we revise the functionality and rmeove this test? |
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.
callout for @eshedmargalit
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, this can happen and I think we want to allow it. Example: you start writing a draft but haven't added authors yet. It will save the draft with authors: ['']
, and we want that to render as
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.
ah okay, we can remove this comment then and keep the test
client/src/components/utils.tsx
Outdated
@@ -46,7 +46,7 @@ export const renderCommaSepList = (items: string[]): JSX.Element[] => | |||
export const removeMiddleAuthors = (authorList: string[], numKeepEitherEnd: number): string[] => { | |||
const numAuthors = authorList.length; | |||
const numKeepTotal = numKeepEitherEnd * 2; | |||
if (numAuthors <= numKeepTotal) { | |||
if (numAuthors <= numKeepTotal || numKeepTotal <= 2) { |
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.
I figured that if this is 0, you just want to keep the whole authorlist? Not sure how you'd want to handle that.
return <h1>{isMounted().toString()}</h1>; | ||
} | ||
|
||
xdescribe('useIsMounted', () => { |
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.
This is skipped because I don't really understand how it works, or how to test it. Any ideas?
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.
I have no idea how to test it either, unless you can stall mounting halfway through the test.
But this hook allows you to ask if the component that uses the hook is still mounted
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.
Approved! I'll wait for your comments on my comments before moving on to #82
client/src/components/utils.test.tsx
Outdated
}); | ||
|
||
describe('shortenAuthors', () => { | ||
// TODO EM: would this ever happen? Can we revise the functionality and rmeove this test? |
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, this can happen and I think we want to allow it. Example: you start writing a draft but haven't added authors yet. It will save the draft with authors: ['']
, and we want that to render as
expectedText: 'N/A', | ||
}, | ||
{ | ||
// TODO EM: is this desired? |
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.
Hmm, no. Cutoff should be strictly >= 0.
client/src/components/utils.test.tsx
Outdated
expectedText: 'testing one two', | ||
}, | ||
{ | ||
description: 'with a string and no cutoff', |
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.
I wouldn't call this "no cutoff"; I'd say the cutoff is 0.
But I do think the cutoff should have to be a natural number
client/src/components/utils.test.tsx
Outdated
}[] = [ | ||
{ | ||
description: 'with no tag', | ||
input: '', | ||
output: 'hsl(0,80%,30%)', | ||
}, | ||
{ | ||
description: 'with some tag', | ||
input: 'i am a tag', | ||
output: 'hsl(-354,80%,30%)', | ||
}, | ||
]; |
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.
Why test these scenarios at all? If it's just a functionality test, the next one covers that, and I don't think we actually care what color the blank string is assigned to.
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.
roger roger, if this isn't important, we can scrap the tests
return <h1>{isMounted().toString()}</h1>; | ||
} | ||
|
||
xdescribe('useIsMounted', () => { |
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.
I have no idea how to test it either, unless you can stall mounting halfway through the test.
But this hook allows you to ask if the component that uses the hook is still mounted
* App tests * Add scaffolding for useIsMounted * Ignore .js, .jsx, and other CRA files * Action creator tests * renderCommaSepList tests * removeMiddleAuthorsTest * shortenAuthors test * shortenTableString * getTagColor test * isDOI test * markdown test * shhhh is okay * self review feedback * CR feebdack
These are basically just tests for
utils.tsx
, but I need @eshedmargalit 's input on some of these.