Skip to content

Commit

Permalink
Added valueComponentProps for DetailsTable for better type checking.
Browse files Browse the repository at this point in the history
  • Loading branch information
@eterna2
eterna2 committed May 9, 2020
1 parent 569a31a commit 540f47c
Show file tree
Hide file tree
Showing 6 changed files with 139 additions and 37 deletions.
2 changes: 1 addition & 1 deletion frontend/src/components/DetailsTable.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -906,7 +906,7 @@ describe('DetailsTable', () => {
<DetailsTable
fields={[['key2', { key: 'foobar', bucket: 'bucket', endpoint: 's3.amazonaws.com' }]]}
valueComponent={ValueComponent}
extraprop='extra'
valueComponentProps={{ extraprop: 'extra' }}
/>,
);
expect(getByTestId('value-component')).toMatchInlineSnapshot(`
Expand Down
9 changes: 5 additions & 4 deletions frontend/src/components/DetailsTable.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,15 @@ interface DetailsTableProps<T> {
fields: Array<KeyValue<string | T>>;
title?: string;
valueComponent?: React.FC<ValueComponentProps<T>>;
[key: string]: any;
valueComponentProps?: { [key: string]: any };
}

function isString(x: any): x is string {
return typeof x === 'string';
}

const DetailsTable = <T extends {}>(props: DetailsTableProps<T>) => {
const { fields, title, valueComponent: ValueComponent, ...rest } = props;
const { fields, title, valueComponent: ValueComponent, valueComponentProps } = props;
return (
<React.Fragment>
{!!title && <div className={commonCss.header}>{title}</div>}
Expand Down Expand Up @@ -103,13 +103,14 @@ const DetailsTable = <T extends {}>(props: DetailsTableProps<T>) => {
// do nothing
}
}
// If value is a S3Artifact render a preview, otherwise just display it as is
// If a ValueComponent and a value is provided, render the value with
// the ValueComponent. Otherwise render the value as a string (empty string if null or undefined).
return (
<div key={i} className={css.row}>
<span className={css.key}>{key}</span>
<span className={css.valueText}>
{ValueComponent && value ? (
<ValueComponent value={value} {...rest} />
<ValueComponent value={value} {...valueComponentProps} />
) : (
`${value || ''}`
)}
Expand Down
141 changes: 113 additions & 28 deletions frontend/src/components/MinioArtifactPreview.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,12 @@ import React from 'react';
import { render, act } from '@testing-library/react';
import MinioArtifactPreview from './MinioArtifactPreview';
import { Apis } from '../lib/Apis';
import TestUtils from '../TestUtils';

jest.mock('../lib/Apis');

describe('MinioArtifactPreview', () => {
const readFile = Apis.readFile as jest.Mock;
const readFile = jest.spyOn(Apis, 'readFile');

beforeEach(() => {
readFile.mockResolvedValue('preview ...');
Expand All @@ -32,17 +33,17 @@ describe('MinioArtifactPreview', () => {
});

it('handles undefined artifact', () => {
const { container } = render(<MinioArtifactPreview artifact={undefined} />);
const { container } = render(<MinioArtifactPreview value={undefined} />);
expect(container).toMatchInlineSnapshot(`<div />`);
});

it('handles null artifact', () => {
const { container } = render(<MinioArtifactPreview artifact={null as any} />);
const { container } = render(<MinioArtifactPreview value={null as any} />);
expect(container).toMatchInlineSnapshot(`<div />`);
});

it('handles empty artifact', () => {
const { container } = render(<MinioArtifactPreview artifact={{} as any} />);
const { container } = render(<MinioArtifactPreview value={{} as any} />);
expect(container).toMatchInlineSnapshot(`<div />`);
});

Expand All @@ -54,7 +55,7 @@ describe('MinioArtifactPreview', () => {
key: 'bar',
secretKeySecret: { key: 'secretkey', optional: false, name: 'minio' },
};
const { container } = render(<MinioArtifactPreview artifact={s3Artifact} />);
const { container } = render(<MinioArtifactPreview value={s3Artifact} />);
expect(container).toMatchInlineSnapshot(`<div />`);
});

Expand All @@ -66,10 +67,28 @@ describe('MinioArtifactPreview', () => {
key: '',
secretKeySecret: { key: 'secretkey', optional: false, name: 'minio' },
};
const { container } = render(<MinioArtifactPreview artifact={s3Artifact} />);
const { container } = render(<MinioArtifactPreview value={s3Artifact} />);
expect(container).toMatchInlineSnapshot(`<div />`);
});

it('handles string value', () => {
const { container } = render(<MinioArtifactPreview value='teststring' />);
expect(container).toMatchInlineSnapshot(`
<div>
teststring
</div>
`);
});

it('handles boolean value', () => {
const { container } = render(<MinioArtifactPreview value={false as any} />);
expect(container).toMatchInlineSnapshot(`
<div>
false
</div>
`);
});

it('handles s3 artifact', async () => {
const s3Artifact = {
accessKeySecret: { key: 'accesskey', optional: false, name: 'minio' },
Expand All @@ -79,12 +98,30 @@ describe('MinioArtifactPreview', () => {
secretKeySecret: { key: 'secretkey', optional: false, name: 'minio' },
};

const container = document.body.appendChild(document.createElement('div'));
await act(async () => {
render(<MinioArtifactPreview artifact={s3Artifact} />, { container });
});

expect(container).toMatchInlineSnapshot(`<div />`);
const { container } = render(<MinioArtifactPreview value={s3Artifact} />);
await act(TestUtils.flushPromises);
expect(container).toMatchInlineSnapshot(`
<div>
<div
class="root"
>
<a
class="link"
rel="noopener"
target="_blank"
/>
<div
class="preview"
>
<small>
<pre>
preview ...
</pre>
</small>
</div>
</div>
</div>
`);
});

it('handles minio artifact', async () => {
Expand All @@ -97,9 +134,30 @@ describe('MinioArtifactPreview', () => {
};
const container = document.body.appendChild(document.createElement('div'));
await act(async () => {
render(<MinioArtifactPreview artifact={minioArtifact} />, { container });
render(<MinioArtifactPreview value={minioArtifact} />, { container });
});
expect(container).toMatchInlineSnapshot(`<div />`);
expect(container).toMatchInlineSnapshot(`
<div>
<div
class="root"
>
<a
class="link"
rel="noopener"
target="_blank"
/>
<div
class="preview"
>
<small>
<pre>
preview ...
</pre>
</small>
</div>
</div>
</div>
`);
});

it('handles artifact with namespace', async () => {
Expand All @@ -110,13 +168,32 @@ describe('MinioArtifactPreview', () => {
key: 'bar',
secretKeySecret: { key: 'secretkey', optional: false, name: 'minio' },
};
const container = document.body.appendChild(document.createElement('div'));
await act(async () => {
render(<MinioArtifactPreview artifact={minioArtifact} namespace='namespace' />, {
container,
});
});
expect(container).toMatchInlineSnapshot(`<div />`);
const { container } = render(
<MinioArtifactPreview value={minioArtifact} namespace='namespace' />,
);
await act(TestUtils.flushPromises);
expect(container).toMatchInlineSnapshot(`
<div>
<div
class="root"
>
<a
class="link"
rel="noopener"
target="_blank"
/>
<div
class="preview"
>
<small>
<pre>
preview ...
</pre>
</small>
</div>
</div>
</div>
`);
});

it('handles artifact cleanly even when fetch fails', async () => {
Expand All @@ -128,12 +205,20 @@ describe('MinioArtifactPreview', () => {
secretKeySecret: { key: 'secretkey', optional: false, name: 'minio' },
};
readFile.mockRejectedValue('unknown error');
const container = document.body.appendChild(document.createElement('div'));
await act(async () => {
render(<MinioArtifactPreview artifact={minioArtifact} />, {
container,
});
});
expect(container).toMatchInlineSnapshot(`<div />`);
const { container } = render(<MinioArtifactPreview value={minioArtifact} />);
await act(TestUtils.flushPromises);
expect(container).toMatchInlineSnapshot(`
<div>
<div
class="root"
>
<a
class="link"
rel="noopener"
target="_blank"
/>
</div>
</div>
`);
});
});
6 changes: 4 additions & 2 deletions frontend/src/components/MinioArtifactPreview.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,10 @@ const MinioArtifactPreview: React.FC<MinioArtifactPreviewProps> = ({
}, [storagePath, peek, namespace]);

if (!storagePath) {
// return value as is if it is defined
return value ? <React.Fragment>{`${value}`}</React.Fragment> : null;
// if value is undefined, null, or an invalid s3artifact object, don't render
if (value === null || value === undefined || typeof value === 'object') return null;
// otherwise render value as string (with default string method)
return <React.Fragment>{`${value}`}</React.Fragment>;
}

// TODO need to come to an agreement how to encode artifact info inside a url
Expand Down
8 changes: 6 additions & 2 deletions frontend/src/pages/RunDetails.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,17 +357,21 @@ class RunDetails extends Page<RunDetailsInternalProps, RunDetailsState> {
<DetailsTable
title='Input artifacts'
fields={inputArtifacts}
namespace={this.state.workflow?.metadata?.namespace}
valueComponent={MinioArtifactPreview}
valueComponentProps={{
namespace: this.state.workflow?.metadata?.namespace,
}}
/>

<DetailsTable title='Output parameters' fields={outputParams} />

<DetailsTable
title='Output artifacts'
fields={outputArtifacts}
namespace={this.state.workflow?.metadata?.namespace}
valueComponent={MinioArtifactPreview}
valueComponentProps={{
namespace: this.state.workflow?.metadata?.namespace,
}}
/>
</div>
)}
Expand Down
10 changes: 10 additions & 0 deletions frontend/src/pages/__snapshots__/RunDetails.test.tsx.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1771,6 +1771,11 @@ exports[`RunDetails switches to inputs/outputs tab in side pane 1`] = `
fields={Array []}
title="Input artifacts"
valueComponent={[Function]}
valueComponentProps={
Object {
"namespace": undefined,
}
}
/>
<DetailsTable
fields={
Expand All @@ -1791,6 +1796,11 @@ exports[`RunDetails switches to inputs/outputs tab in side pane 1`] = `
fields={Array []}
title="Output artifacts"
valueComponent={[Function]}
valueComponentProps={
Object {
"namespace": undefined,
}
}
/>
</div>
</div>
Expand Down

0 comments on commit 540f47c

Please sign in to comment.