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

[SCHEMATIC-208] fix slow test times #1566

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

[SCHEMATIC-208] fix slow test times #1566

wants to merge 3 commits into from

Conversation

andrewelamb
Copy link
Contributor

@andrewelamb andrewelamb commented Jan 8, 2025 •
edited by jira bot
Loading

  • Fixes SCHEMATIC-208
  • Removes test_create_table
  • Lowers test folders to copy in test manifest

@andrewelamb andrewelamb requested a review from a team as a code owner January 8, 2025 17:14
@thomasyu888 thomasyu888 changed the title SCHEMATIC 208 fix slow test times [SCHEMATIC-208] fix slow test times Jan 8, 2025
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Jan 8, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@BryanFauble
Copy link
Collaborator

BryanFauble commented Jan 8, 2025 •
edited
Loading

I was hoping to be able to look at code coverage before/after the changes, but it seems that code coverage in sonarcloud broke in December:

https://sonarcloud.io/project/activity?from=2024-09-05T10%3A25%3A26%2B0000&graph=coverage&id=Sage-Bionetworks_schematic&to=2025-01-04T03%3A53%3A46%2B0000
image

If you have time while working on these changes getting code coverage working again would be amazing!

Code coverage works by passing an XML file to sonarcloud based on this github workflow code:

schematic/.github/workflows/test.yml

Lines 191 to 204 in 48da1c0

- name: Download coverage report
uses: actions/download-artifact@v4
if: steps.check_coverage_report.outputs.exists == 'true'
with:
name: coverage-report
- name: Check coverage.xml file existence
id: check_coverage_xml
uses: andstor/file-existence-action@v3
with:
files: "coverage.xml"
# This is a workaround described in https://community.sonarsource.com/t/sonar-on-github-actions-with-python-coverage-source-issue/36057
- name: Override Coverage Source Path for Sonar
if: steps.check_coverage_xml.outputs.files_exists == 'true'
run: sed -i "s/<source>\/home\/runner\/work\/schematic\/schematic\/schematic<\/source>/<source>\/github\/workspace\/schematic<\/source>/g" coverage.xml

BryanFauble
BryanFauble approved these changes Jan 8, 2025
View reviewed changes
Copy link
Collaborator

@BryanFauble BryanFauble left a comment

Choose a reason for hiding this comment

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

Looks good to me! I would like to see # of covered lines before/after the changes though (See my comment)

GiaJordan
GiaJordan reviewed Jan 8, 2025
View reviewed changes
tests/data/mock_manifests/table_manifest.csv
@@ -1,10 +1,3 @@
Component,Days to Follow Up,Adverse Event,Progression or Recurrence,Barretts Esophagus Goblet Cells Present,BMI,Cause of Response,Comorbidity,Comorbidity Method of Diagnosis,Days to Adverse Event,Days to Comorbidity,Diabetes Treatment Type,Disease Response,DLCO Ref Predictive Percent,ECOG Performance Status,FEV1 FVC Post Bronch Percent,FEV 1 FVC Pre Bronch Percent,FEV1 Ref Post Bronch Percent,FEV1 Ref Pre Bronch Percent,Height,Hepatitis Sustained Virological Response,HPV Positive Type,Karnofsky Performance Status,Menopause Status,Pancreatitis Onset Year,Reflux Treatment Type,Risk Factor,Risk Factor Treatment,Viral Hepatitis Serologies,Weight,Days to Progression,Days to Progression Free,Days to Recurrence,Progression or Recurrence Anatomic Site,Progression or Recurrence Type,entityId,HTAN Participant ID
FollowUp,73.0,,Not Reported,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,syn22249015,455.0
FollowUp,73.0,,Not Reported,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,syn22249915,456.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Were these entities deleted on synapse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They were not, they are in an HTAN project.

GiaJordan
GiaJordan approved these changes Jan 8, 2025
View reviewed changes
Copy link
Contributor

@GiaJordan GiaJordan left a comment

Choose a reason for hiding this comment

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

LGTM! I know you mentioned also removing the parameterization from the remaining tests; does that need to be done in a later PR?

@thomasyu888
Copy link
Member

Looks good to me! I would like to see # of covered lines before/after the changes though (See my comment)

Upvoting this. Thanks everyone!

@linglp linglp self-requested a review January 8, 2025 20:23
linglp
linglp approved these changes Jan 8, 2025
View reviewed changes
Copy link
Contributor

@linglp linglp left a comment

Choose a reason for hiding this comment

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

Looks good to me too but also likes to see code coverage.

@andrewelamb
Copy link
Contributor Author

So somewhere around here, the Sonarcloud cation became depricated, and also stopped working. I'm goign to try updating to the suggested action and see if that works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GiaJordan GiaJordan GiaJordan approved these changes

@linglp linglp linglp approved these changes

@BryanFauble BryanFauble BryanFauble approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@andrewelamb @BryanFauble @thomasyu888 @linglp @GiaJordan