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.

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

chore: add e2e tests for cellguide cxgs and related features #902

Merged
merged 13 commits into from
Apr 29, 2024

Conversation

atarashansky
Copy link
Contributor

@atarashansky atarashansky commented Apr 26, 2024

Adding E2E tests for the following interactions:

  • Assert no author categories or standard categories - done
  • Marker Gene Sets is visible and collapsed. Expansion reveals expected gene sets - done
  • Gene sets are alphabetically sorted. - done
  • Gene set expansion works as expected. - done
  • Gene set modification/deletion and page refresh resets the gene set - done
  • Categorical labels are sorted in descending order by mean expression when genesets are colored by. - done
  • Labels with 0 cells after subsetting are filtered out. - done

Added a test fixture in the cellguide-cxgs/ path to set isCellGuideCxg equal to true.

Note that because we now have test coverage for the new categorical label behaviors, I removed the isCellGuideCxg feature flag from it. Now all explorer instances will benefit from the improved UX. Namely, the improvements are:

  • Remove labels with 0 cell counts when subsetting
  • Sort labels in descending order when coloring by gene expression or any other continuous metadata.

Also, to reduce the size of the new test fixture added (example.cxg), I set the gene expression tiledb arrays Xr and Xc to have no data in it. This is fine as none of the cellguide-specific tests require gene expression data.

@atarashansky atarashansky requested a review from tihuan April 26, 2024 20:19
Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.27%. Comparing base (0d204de) to head (54d97e3).
Report is 31 commits behind head on main.

❗ Current head 54d97e3 differs from pull request most recent head 0ae3b18. Consider uploading reports for the commit 0ae3b18 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #902      +/-   ##
==========================================
- Coverage   77.47%   77.27%   -0.21%     
==========================================
  Files          88       45      -43     
  Lines        6856     3512    -3344     
==========================================
- Hits         5312     2714    -2598     
+ Misses       1544      798     -746     
Flag Coverage Δ
frontend ?
javascript ?
smokeTest ?
unitTest ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

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

Looks really good! Thanks for adding comprehensive tests, Alec 🙏 Some v minor comments otherwise LGTM 🔥 !!

client/__tests__/e2e/cellxgeneActions.ts Outdated Show resolved Hide resolved
client/__tests__/e2e/cellxgeneActions.ts Show resolved Hide resolved
client/__tests__/e2e/e2e.test.ts Outdated Show resolved Hide resolved
@@ -101,6 +104,143 @@ const testURLs = {
"super-cool-spatial.cxg": pageURLSpatial,
};

describe(`Testing CellGuideCXG at ${pageURLCellGuide}`, () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should put the CellGuideCXG test in its own test file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to config playwright to run the separate file - if you think it's important, perhaps you could commit it to this branch so I can learn? 👼

client/__tests__/e2e/e2e.test.ts Outdated Show resolved Hide resolved
client/__tests__/e2e/e2e.test.ts Outdated Show resolved Hide resolved
@atarashansky atarashansky requested a review from tihuan April 29, 2024 18:02
Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

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

LGTMMM! Thanks so much again, Alec 👏 🙏 🔥

@atarashansky atarashansky enabled auto-merge (squash) April 29, 2024 21:32
@atarashansky atarashansky disabled auto-merge April 29, 2024 22:31
@atarashansky atarashansky merged commit be168f7 into main Apr 29, 2024
3 of 5 checks passed
@atarashansky atarashansky deleted the atar-cg-tests branch April 29, 2024 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants