Skip to content
This repository has been archived by the owner on Jun 18, 2019. It is now read-only.

Copy shares with Electron's Clipboard API #34

Merged
merged 3 commits into from
Jun 15, 2017

Conversation

garrettr
Copy link
Contributor

@garrettr garrettr commented Jun 9, 2017

This is an alternative to #33 for fixing #29. I think this is a cleaner solution and would prefer it to #33 as long as there are no potential issues with Electron's Clipboard API that I am unaware of.

Using the Clipboard API simplifies the error handling when copying shares because while document.execCommand('copy') can either return a failure status or raise an exception, clipboard.writeText is (as far as I can tell) guaranteed to succeed.

@garrettr garrettr requested a review from GabeIsman June 9, 2017 08:07
Copy link
Contributor

@GabeIsman GabeIsman left a comment

Choose a reason for hiding this comment

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

As I mentioned in #33, this will cause npm run test-e2e to fail. The "find and store the share values" step relies on these hidden inputs. It certainly doesn't have to, but we'll need to come up with an alternate solution for extracting the shares.

@garrettr
Copy link
Contributor Author

garrettr commented Jun 9, 2017

@GabeIsman I'll definitely block merging this on making sure the tests are passing. Once I have #31 resolved, I should be able to at least to attempt to fix the tests to work with this approach; as you pointed out, we may also want to implement #35 to improve the testing story, both for this specific case and more generally.

@garrettr garrettr self-assigned this Jun 14, 2017
@garrettr
Copy link
Contributor Author

This is waiting on #39, which will allow us to properly test clipboard interactions with the Electron API.

Now that we've integrated Spectron, we can access Electron API's
directly from within the e2e tests, which makes testing
clipboard-related functionality straightforward.

I added the className to CopyButton and unique id's to each ShareRow to
make it easier to construct an explicit WebdriverIO selector. I think
explicit selectors (and correspondingly, careful design of classes and
ID's) are good because they make tests easier to read and reason about,
and reduce the likelihood of test failures due to imprecise selectors.
@garrettr garrettr force-pushed the copy-shares-with-electron-clipboard-api branch from e098419 to 6120899 Compare June 14, 2017 23:28
@garrettr
Copy link
Contributor Author

Merged #39, rebased this on top of master, fixed the e2e tests (which was easy as pie thanks to Spectron), and added an additional bonus test for copying the recovered secret at the end of the recovery flow.

@garrettr garrettr requested a review from GabeIsman June 14, 2017 23:31
@@ -1,4 +1,5 @@
import React, { Component } from 'react';
const {clipboard} = require('electron');
Copy link
Contributor

Choose a reason for hiding this comment

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

import { clipboard } from 'electron'; for consistency.

await app.client.element(`#share-${i+1} .copy`).click();
const share = await app.electron.clipboard.readText();
shares.push(share);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

😍 So clean!

@garrettr garrettr merged commit e3fdfec into master Jun 15, 2017
@garrettr garrettr deleted the copy-shares-with-electron-clipboard-api branch June 15, 2017 16:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants