Increase test coverage #150
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
andresmgot
requested review from
fridgepoet and
sarahzinger
and removed request for
a team
sarahzinger
approved these changes
Mar 18, 2022
src/appendFrames.test.ts
Outdated
| expect(appendMatchingFrames([], [])).toEqual([]); | ||
| }); | ||
|
|
||
| it('should return input MutableDataFrames', () => { |
There was a problem hiding this comment.
kind of a nit that we can ignore if we don't have good ideas for it:
- in reading this test and the one below I'm not sure I really understand the difference between these two tests. I wonder if we can update the "it" statements to better describe what we're handling or something. Or if these some data frames are mutable should we change them and see that append still handles them the same or something?
There was a problem hiding this comment.
What about :
Suggested change
| it('should return input MutableDataFrames', () => { | |
| it('should return input when input is a MutableDataFrames array, () => { |
and next suggestion?
There was a problem hiding this comment.
sounds good to me
src/appendFrames.ts
Outdated
| @@ -58,6 +58,8 @@ export function appendMatchingFrames(prev: DataFrame[], b: DataFrame[]): DataFra | |||
| for (let i = 0; i < f.length; i++) { | |||
| for (let idx = 0; idx < old.fields.length; idx++) { | |||
| old.fields[idx].values.add(f.fields[idx].values.get(i)); | |||
| // FIXME: I assume this is meant to modify `out` but it's | |||
| // not doing anything atm | |||
There was a problem hiding this comment.
lol should we make a ticket or something?
There was a problem hiding this comment.
double checking this it actually works 🤯
I assume that old.fields[idx] is pointing to the same vector reference than out so modifying that modifies the result. I have added a test that covers this.
yaelleC
reviewed
Mar 18, 2022
src/appendFrames.test.ts
Outdated
| expect(appendMatchingFrames([frame], [])).toEqual([frame]); | ||
| }); | ||
|
|
||
| it('should return input DataFrame', () => { |
There was a problem hiding this comment.
Suggested change
| it('should return input DataFrame', () => { | |
| it('should return a MutableDataFrame version of input when input is a DataFrames array', () => { |
yaelleC
reviewed
Mar 18, 2022
|
|
||
| it('should append new frames', () => { | ||
| const frame = new MutableDataFrame({ fields: [{ name: 'foo', values: v }] }); | ||
| expect(appendMatchingFrames([], [frame])).toEqual([frame]); |
There was a problem hiding this comment.
Should we have a test for two not empty arrays?
Not sure the following works ;)
Suggested change
| expect(appendMatchingFrames([], [frame])).toEqual([frame]); | |
| expect(appendMatchingFrames([frame], [frame])).toEqual([frame, frame]); |
yaelleC
reviewed
Mar 18, 2022
|
|
||
| import { DataSource } from '../DataSource'; | ||
| import { TimestreamOptions, TimestreamQuery } from '../types'; | ||
|
|
||
| export type Props = MetadataInspectorProps<DataSource, TimestreamQuery, TimestreamOptions>; | ||
|
|
||
| export class MetaInspector extends PureComponent<Props> { | ||
| state = { index: 0 }; | ||
| export function MetaInspector(props: Props) { |
yaelleC
approved these changes
Mar 21, 2022
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.
Fixes #149
appendFrames(and discovered a potential bug?)MetadataInspector(and migrated to a function component)FormsThe coverage is now 47% which is not great but at least covers the 40% level that we wanted: