Skip to content

Test for ExaminePage, fix two bugs it found.#294

Merged
ihodes merged 3 commits intomasterfrom
moar-tests
Nov 26, 2014
Merged

Test for ExaminePage, fix two bugs it found.#294
ihodes merged 3 commits intomasterfrom
moar-tests

Conversation

@danvk
Copy link
Contributor

@danvk danvk commented Nov 25, 2014

This constructs /contigs, /spec and /genotypes responses from a VCF file.

The two bugs were:

  1. The initial request for /genotypes was being throttled, which artificially slowed page load by 500ms.
  2. The exploded rows from multiple samples were being dropped.

I also deleted some code that wasn't being used to boost our code coverage. I suspect that if/when we revive this code, it will be substantially different. So deleting is not a big loss. But feel free to disagree.

This PR bumps our test coverage from 25%→53%.

@tavinathanson
Copy link
Member

Well, that solves the code quality issue w.r.t. bar charts! 💇

Copy link
Member

Choose a reason for hiding this comment

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

The difference between these two methods isn't too clear from the names! I remember being unclear between the two when I was doing a RecordStore read-through.

I realize we don't want a super long name for a method that's called a bunch of places, but I'm still putting in a vote for updateGenotypesThrottled.

Copy link
Member

Choose a reason for hiding this comment

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

Also, it looks like the delay is due to debounce rather than throttle? (Not that I'm suggesting you should use throttle, here.)

Copy link
Member

Choose a reason for hiding this comment

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

True about delay being due to debounce rather than throttle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the comment to "debounce".

@danvk
Copy link
Contributor Author

danvk commented Nov 25, 2014

Only sorta solves the code quality problem -- that was just one of two copies of d3.bar-chart.js in this repo :(

Copy link
Member

Choose a reason for hiding this comment

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

So what is this giving us?

Copy link
Member

Choose a reason for hiding this comment

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

The key needs to be unique on all records; previously, if there were multiple samples in the VCF, the keys wouldn't be unique (since we "explode" each VCF row into N genotype rows (N = # of samples)). This fixes that, so we don't drop rows.

Copy link
Member

Choose a reason for hiding this comment

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

May be better to _.pick(record, 'contig', 'position', …).join()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_.pick returns an object, so I'd also have to do _.values. It winds up being longer than this is. I can change if you prefer, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isaac -- can you clarify why REF/ALT are in the key? I don't understand why they're necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Shoot, thought it returned a list. Do with it what you will! (Wouldn't suggest making it longer, though!)

Copy link
Member

Choose a reason for hiding this comment

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

REF/ALT are necessary to make sure the variant is unique--you can have different variants at the same chromosome/position. It's still not necessarily totally theoretically unique, but it gets us closer, within reason.

@danvk
Copy link
Contributor Author

danvk commented Nov 26, 2014

Addressed all comments & rebased.

ihodes added a commit that referenced this pull request Nov 26, 2014
Test for ExaminePage, fix two bugs it found.
@ihodes ihodes merged commit af3bc48 into master Nov 26, 2014
@ihodes ihodes deleted the moar-tests branch December 4, 2014 22:52
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.

3 participants