EjectButton component test

[AWolf81]

Related Issue:
#309

Summary:
Slightly modified EjectButton with two named exports dialogOptions and dialogCallback so they can be used in the test.

Tests:

• Rendering (running and not running) with snapshot tests
• Test dialog
  • Dialog displayed
  • Confirm
  • Dismiss

[AWolf81]

Nit: Maybe I'm inlining this in the render method as this got really short with the exports.

[codecov]

Codecov Report

Merging #348 into master will increase coverage by 0.44%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #348      +/-   ##
==========================================
+ Coverage   24.91%   25.35%   +0.44%     
==========================================
  Files         152      152              
  Lines        3625     3613      -12     
  Branches      388      388              
==========================================
+ Hits          903      916      +13     
+ Misses       2450     2426      -24     
+ Partials      272      271       -1

Impacted Files	Coverage Δ
src/components/EjectButton/EjectButton.js	100% <100%> (+100%)	⬆️
...omponents/BigClickableButton/BigClickableButton.js	20% <0%> (+20%)	⬆️

[superhawk610]

Hey @AWolf81, long time no see 😃

I don't feel comfortable Approving this PR since I've been away from the project for a while, but it looks good to me 👍

My only feedback is that I always avoid func.bind(context) at all costs, but I believe this is one of those rare cases where it's required. The tests are thorough as well.

[AWolf81]

@superhawk610 hey, no problem.

I also always try to avoid bind as arrow functions are usually more readable but here I think it's OK as it's shorter than response => dialogCallback(response). But binding should be also OK here. I just replaced the mentioned arrow function as it was marked as uncovered in the coverage report.

I first tried to mock the dialog.showMessagebox but I wasn't sure how I could test and mock the callback.

[melanieseltzer]

This is great 👍 Just one suggestion for clearer test description but it's not super important to be blocking.

[melanieseltzer]

At first glance it was a little unclear to me what running/not running meant, until I realized it was the pressed state... I think it could be changed to something like:

it('should render without being pressed (not running)', () => {

it('should render as pressed (running)', () => {

[AWolf81]

Good catch. I modified it.

[melanieseltzer]

I didn't realize they now include type definitions 👍

[AWolf81]

Oh, OK, I've tested if the definition is working (by adding a key that's not in the definition - wasn't working) and noticed that this is a TypeScript definition and not Flow.
That's why I removed it.
So there is typing in Electron but no Flow types.

[melanieseltzer]

Do you think we should convert the other dialogs to this format without the handleClick? e.g. in DeleteDependencyButton.js

[AWolf81]

@melanieseltzer yes, the test for that component will be similar to the EjectButton. I think it's also OK to inline the handleClick just add the logic before calling () => dependencyStatus === 'idle' && dialog.showMessageBox(options, callback.bind(this)) and in the callback destructure the required props.

If you like you can create a DeleteDependencyButton.test.js and export the dialogOptions & callback.

[melanieseltzer]

👍 I'll take a stab at converting the rest of the dialogs when I have some free time, I think it'll be good to have everything uniform.