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.

Sign up for GitHub

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

Jump to bottom

Add custom filter tests with new samples and assertResponse fields #4998

Closed
wants to merge 8 commits into from
Closed

Add custom filter tests with new samples and assertResponse fields #4998

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1aee750
add custom filter tests with new samples and assertResponse fields
Sep 13, 2024
615bb91
add assertResponse handler
Sep 13, 2024
15e4b60
fix validation
Sep 16, 2024
8b5d387
update bin counts tests to include samples and assertResponse fields
Sep 17, 2024
32a4faf
delete specs
Sep 18, 2024
d355c0e
delete merge api test from webpack
Sep 19, 2024
15780a2
address review comments
Sep 26, 2024
a5a5932
remove assertResponse dependence for clickhouse fetch
Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3,581 changes: 0 additions & 3,581 deletions apiTests/specs/clinical-data-filters.json
View file
Open in desktop

This file was deleted.

204 changes: 0 additions & 204 deletions apiTests/specs/custom-data-bin-counts-filter.json
View file
Open in desktop

This file was deleted.

204 changes: 0 additions & 204 deletions apiTests/specs/custom-data-counts-filter.json
View file
Open in desktop

This file was deleted.

126 changes: 0 additions & 126 deletions apiTests/specs/generic-assay-data-bin-counts.json
View file
Open in desktop

This file was deleted.

10 changes: 0 additions & 10 deletions apiTests/specs/genie-public.json
View file
Open in desktop

This file was deleted.

150 changes: 0 additions & 150 deletions apiTests/specs/genomic-data-bin-counts.json
View file
Open in desktop

This file was deleted.

144 changes: 0 additions & 144 deletions apiTests/specs/mutation-data-counts.json
View file
Open in desktop

This file was deleted.

169 changes: 0 additions & 169 deletions apiTests/specs/sample-lists-counts-filter.json
View file
Open in desktop

This file was deleted.

5 changes: 4 additions & 1 deletion src/pages/studyView/rfc80Tester.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,10 @@ export const RFC80Test = observer(function() {
test.url,
test.data,
test.label,
test.hash
test.hash,
undefined,
undefined,
col.assertResponse
).then((report: any) => {
report.test = test;
place = place + 1;
Expand Down
107 changes: 76 additions & 31 deletions src/shared/api/validation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,8 @@ export function validate(
label: string,
hash: number,
body?: any,
elapsedTime?: any
elapsedTime?: any,
assertResponse?: any[]
) {
const clStart = performance.now();
let chDuration: number, legacyDuration: number;
Expand All @@ -258,39 +259,75 @@ export function validate(
if (body) {
chXHR = Promise.resolve({ body, elapsedTime });
} else {
chXHR = $.ajax({
method: 'post',
url: url,
data: JSON.stringify(params),
contentType: 'application/json',
}).then((body, state, xhr) => {
return { body, elapsedTime: xhr.getResponseHeader('elapsed-time') };
});
}

return chXHR
.then(({ body, elapsedTime }: any) => {
let legacyUrl = url.replace(/column-store\//, '');
if (treatmentLegacyUrl[label]) {
legacyUrl = treatmentLegacyUrl[label](legacyUrl);
if (assertResponse && assertResponse[0] && assertResponse[0].attributeId) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure i understand the necessity of this code up here. the clickhouse fetch should be entirely independent of the assertResponse, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i used it to get the one attribute that is used in assertResponse. so the clinical data counts clickhouse fetch only fetches data for that attribute. then its an easy compare between that response and assertResponse.

let data: any = {};
data['attributes'] = [{ attributeId: assertResponse[0].attributeId }];
if (params.studyViewFilter) {
const {
attributes,
...filteredStudyViewFilter
} = params.studyViewFilter;
data['studyViewFilter'] = filteredStudyViewFilter;
} else {
data['studyViewFilter'] = params;
}
const legacyXHR = $.ajax({
chXHR = $.ajax({
method: 'post',
url: legacyUrl,
url: '/api/column-store/clinical-data-counts/fetch?',
data: JSON.stringify(data),
contentType: 'application/json',
}).then((body, state, xhr) => {
return { body, elapsedTime: xhr.getResponseHeader('elapsed-time') };
});
} else {
chXHR = $.ajax({
method: 'post',
url: url,
data: JSON.stringify(params),
contentType: 'application/json',
}).then((body, state, xhr) => {
return { body, elapsedTime: xhr.getResponseHeader('elapsed-time') };
});
return legacyXHR.then(legacyResult => {
const result: any = compareCounts(body, legacyResult, label);
}
}

return chXHR
alisman marked this conversation as resolved.
Show resolved Hide resolved
.then(({ body, elapsedTime }: any) => {
if (!assertResponse) {
let legacyUrl = url.replace(/column-store\//, '');
if (treatmentLegacyUrl[label]) {
legacyUrl = treatmentLegacyUrl[label](legacyUrl);
}
const legacyXHR = $.ajax({
method: 'post',
url: legacyUrl,
data: JSON.stringify(params),
contentType: 'application/json',
});
return legacyXHR.then(legacyResult => {
const result: any = compareCounts(body, legacyResult, label);
result.url = url;
result.hash = hash;
result.data = params;
result.chDuration = parseFloat(elapsedTime);
result.legacyDuration = parseFloat(
legacyXHR.getResponseHeader('elapsed-time') || ''
);
return result;
});
}
else {
const result: any = compareCounts(
alisman marked this conversation as resolved.
Show resolved Hide resolved
body,
assertResponse,
'ClinicalDataCounts'
);
result.url = url;
result.hash = hash;
result.data = params;
result.chDuration = parseFloat(elapsedTime);
result.legacyDuration = parseFloat(
legacyXHR.getResponseHeader('elapsed-time') || ''
);
return result;
});
}
})
.catch(() => {
const result: any = {};
Expand Down Expand Up @@ -324,13 +361,21 @@ export function reportValidationResult(result: any, prefix = '') {
});

result.status &&
console.log(
`${prefix} ${result.label} (${
result.hash
}) passed :) ch: ${result.chDuration.toFixed(
0
)} legacy: ${result.legacyDuration.toFixed(0)}`
);
(!result.legacyDuration
Copy link
Collaborator

Choose a reason for hiding this comment

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

try to avoid these duplicating logic (console.log(...)) like this for only a "little" thing like whether it has legacyDuration. better to introduce a variable that gets always gets printed and can sometimes be blank (e.g. when there's no legacyDuration).

Copy link
Collaborator

Choose a reason for hiding this comment

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

i do think it's a good idea to include the fact that there's an assertResponse in this console log here.

? console.log(
`${prefix} ${result.label} (${
result.hash
}) assertResponse detected, passed :) ch: ${result.chDuration.toFixed(
0
)}`
)
: console.log(
`${prefix} ${result.label} (${
result.hash
}) passed :) ch: ${result.chDuration.toFixed(
0
)} legacy: ${result.legacyDuration.toFixed(0)}`
));

if (!result.status) {
_.forEach(result.clDataSorted, (cl: any, i: number) => {
Expand Down
10 changes: 0 additions & 10 deletions webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,14 +599,4 @@ if (isTest) {
}
// End Testing

mergeApiTestJson();

if (isDev) {
watch('./apiTests/specs', async function(event, filename) {
if (event === 'change') {
mergeApiTestJson();
}
});
}

module.exports = config;
Loading