Skip to content

Chore/test audio bars#14

Merged
zzarcon merged 2 commits intomasterfrom
chore/test-audio-bars
Jun 1, 2017
Merged

Chore/test audio bars#14
zzarcon merged 2 commits intomasterfrom
chore/test-audio-bars

Conversation

@majames
Copy link
Copy Markdown
Collaborator

@majames majames commented May 27, 2017

Test (allthethings)

@majames majames requested a review from zzarcon May 27, 2017 00:56
@coveralls
Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling 688c9ee699591942502b5c36e90905cca5a55619 on chore/test-audio-bars into ** on master**.

@majames majames force-pushed the chore/test-audio-bars branch from 688c9ee to 03c863d Compare May 27, 2017 01:04
@coveralls
Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling 03c863d9aff17fb92d62cfb389283b6b8b60baf2 on chore/test-audio-bars into ** on master**.

@majames majames requested a review from rpunkfu May 27, 2017 01:07
Comment thread README.md Outdated
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Update badges to point at devlucky

Comment thread src/analyser/index.tsx Outdated
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Move logic for bucketing frequency data into Analyser (other visualisations can use this)

Comment thread src/bars/index.test.tsx Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Those cases are gold!

Comment thread src/bars/index.test.tsx Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure if this it is giving us some value or we are getting to deep here. Like from the outside, seeing 0 , 0, 400, 400 looks really weird

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. I defs think it looks a little weird... but I do like testing the logic for clearing the canvas.

How about I use explaining variables? Like:

const canvasDefaultWidth = 400;
const canvasDefaultHeight = 400;

Comment thread src/bars/index.test.tsx Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same thing here as before, I do like the draws the correct number of bars to the canvas test and I think we should keep it, but I will prefer to see something like:

expect(wrapper.instance().drawSingleBar).toHaveBeenCalledTimes(expectedNumberOfDrawedBars);

What do you think? I don't think doing such calculations in the testsuite is the way to go, I think we should try to abstract that more

Comment thread src/bars/index.tsx Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice one

@majames majames force-pushed the chore/test-audio-bars branch from 03c863d to b49a1f9 Compare May 29, 2017 10:01
@zzarcon zzarcon merged commit 16821bc into master Jun 1, 2017
@zzarcon zzarcon deleted the chore/test-audio-bars branch June 1, 2017 06:26
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