Skip to content

Conversation

@whymarrh
Copy link
Contributor

@whymarrh whymarrh commented Feb 27, 2019

Closes #6154

This PR adds the ability for us to write e2e tests that use fixture data for both the extension's background state and the current state of the blockchain (via Ganache). Fortunately we compute the initial Redux store contents (the front-end state) from the initial background state so loading the background state from a fixture is currently synonymous with loading the Redux state.

Overview

As outlined in #6154:

We need to create infrastructure and tooling that allows us to run our e2e tests from an existing state.

We need a solution for the background that allows us to dynamically load state during test runs.

Once this task is complete, our e2e tests will be able to start the app at a state given in [JSON] file (plus [Ganache]) and run tests from there.

This PR introduces a way to write and organize fixtures into a set of directories that contain both the background and Ganache state as well as a load function that takes a path to a fixture directory, loads that state and runs a set of assertions in that context.

By example:

describe('MetaMask Browser Extension from existing state', function () {
  it('displays the lock screen', async () => {
    await load('onboarded', async ({driver}) => {
      const button = await driver.findElement('.unlock-page__title')
      await button.click()
    })
  })
})

This approach allows us to start the app in a given state and make assertions from there. The implementation (detailed below) allows us to execute each of the tests in parallel. The arguments passed to the callback currently include a wrapper around a WebDriver instance that exposes helper functions similar to those we've already been using (e.g. functions handling timeouts and retries). The helper functions on the WebDriver wrapper can be expanded to include any sugar/convenience methods we'd like (see also #6155, #6156).

The implementation

The background loads its state using the LocalStore wrapper for the storage.local API:

// read from disk
// first from preferred, async API:
versionedData = (await localStore.get()) ||
diskStore.getState() ||
migrator.generateInitialState(firstTimeState)

Background

The e2e tests now spin up a local HTTP server that provides fixture state to clients at pre-determined URLs. The extension now has an e2e flag set at build time that is used to set the application down e2e-specific flows which we're using in the background to load the initial state instead from the network (from said server). This setup keeps everything local but does introduce network latency where there wasn't any before only during e2e tests (a small cost). When built without the flag set the application behaves no differently.

Each time the load function is called the fixture server is pointed at a new fixture directory. This requires that the startup of each test be sequential but allows the tests to run in parallel after the initial state is loaded (i.e. all of the assertions, the majority of the test).

Ganache

Instead of using ganache-cli from the command line, the e2e tests now start ganache's server during each load invocation. At the same time the tests spin up a local server for the fixture data, we also start a ganache-cli server instance with the options outlined in the ${fixturesDirectory}/db.js file (note that this is a JS file instead of JSON to be able to things like path.resolve). Once the server is up and running, we can interact with the RPC endpoint. Because the server is started on any available port, a ganache reference is passed through to the tests to allow them to setup the RPC endpoint as needed.

The state of this PR (tasks)

The following task list will be updated as this PR is updated:

  • Ability to load background state in
    • Chrome
    • Firefox
  • Ability to load Ganache state

Testing this PR (no pun intended)

This PR includes an example spec file (e2e/tests/example.spec.js) which outlines the API future tests can be written with. (It might be deleted before this is merged?) To run the example spec file:

Firefox (the default browser):

npx mocha --no-timeouts e2e/tests/example.spec.js

Firefox, explicitly:

BROWSER=firefox npx mocha --no-timeouts e2e/tests/example.spec.js

Chrome:

BROWSER=chrome npx mocha --no-timeouts e2e/tests/example.spec.js

Alternative approaches to pre-loading background state

Briefly on the alternative approaches we've considered for this functionality:

Editing the extension source files before loading it

(A recap of a discussion in Slack.)

The old e2e tests did mutate background state by prepending the background file with a global variable assignment, setting the METAMASK_TEST_CONFIG:

async function createModifiedTestBuild ({ browser, srcPath }) {
// copy build to test-builds directory
const extPath = path.resolve(`test-builds/${browser}`)
await fs.ensureDir(extPath)
await fs.copy(srcPath, extPath)
// inject METAMASK_TEST_CONFIG setting default test network
const config = { NetworkController: { provider: { type: 'localhost' } } }
await prependFile(`${extPath}/background.js`, `window.METAMASK_TEST_CONFIG=${JSON.stringify(config)};\n`)
return { extPath }
}

This approach doesn't support the ability for us to run tests in parallel. We could update it to include writing out a separate build of the extension to disk N times for N tests and edit each differently but that would be a lot of I/O during what is already an already slow test suite.

Preloading the extension state before loading the extension

The ideal approach to loading background would be to have the browser know about the state before the extension is installed. This is easily possible with Firefox but less easy to do with Chrome. Firefox stores extension state in a single text file on disk whereas Chrome stores its extension state across multiple binary leveldb databases.[3]

@danjm danjm self-assigned this Feb 27, 2019
@kumavis
Copy link
Member

kumavis commented Feb 27, 2019

We have an API available for testing/tooling in the ui set the locale, could use it to override the state.

If we give it more powerful features like that we should consider deleting the api object after some time out, so it can only be used at boot and not abused at runtime

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an OS, and possibly individual system, dependent path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@kumavis kumavis mentioned this pull request Mar 12, 2019
4 tasks
@whymarrh whymarrh force-pushed the e2e-load-fixtures branch 4 times, most recently from a651560 to a206ded Compare March 13, 2019 16:48
@whymarrh
Copy link
Contributor Author

This PR currently supports the ability to load background state in both Firefox and Chrome

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be conditional on some environment variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I think this can be merged in to the destination branch (e2e-2.0) for now

Copy link
Contributor

Choose a reason for hiding this comment

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

But I think this can be merged in to the destination branch (e2e-2.0) for now

We should create an issue or some sort of hard reminder to fix this before merging the destination branch to develop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #6324

@whymarrh whymarrh force-pushed the e2e-load-fixtures branch from a206ded to 77eb21a Compare March 14, 2019 02:19
@whymarrh
Copy link
Contributor Author

77eb21a adds the ability to start Ganache from options (which includes an existing db path) during e2e tests. The Ganache wrapper (i.e., helper functions) hasn't been fleshed out yet since I don't have a good sense of what its API should be—that'll be something that comes out of #6157 as well. What we can do is interact with Ganache's RPC endpoint on some port.

@whymarrh whymarrh marked this pull request as ready for review March 14, 2019 02:25
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this do nothing but return an empty promise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it's ephemeral—during the lifetime of the app we don't need to bother saving the state that's set here

ui/app/store.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be treated as true in all cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, removed. Good catch, this was for testing.

ui/index.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we definitely want 'debug' in all cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, removed. Good catch, this was for testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can 'http://localhost:12345/state.json' be specified dynamically, from an env variable or at least stored in a descriptive constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll extract this into a constant, good point. I think I'll leave this hard-coded though as this is something that we need to share and keep the same between the app (which could be pre-built into dist/) and the e2e logic. It's possible to have it dynamically set but I feel like this is easy enough to start with and should we need to we can make it dynamic later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can these properties be specified dynamically, from an env variable or at least stored in a descriptive constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Constants for these as well

e2e/index.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the reference counter? Is it to handle multiple simultaneous calls of the fixture server, such that only one instance of the server can ever be running at a time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. We can only have (and only need) the single server on port 12345 so we want to make sure it's only started once and that it's shutdown when the tests end. I'm open to better ways of doing this.

e2e/index.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we give load a slightly more unique name so that it is easy to grep for its uses in the codebase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's... a really good suggestion. I'll do withFixtures?

The constants are mirrored between the e2e logic (`e2e/`) and the app logic
in the ReadOnlyNetworkStore (`app/scripts/lib/network-store.js`) as they need
to be the same in both places. These could eventually be made dynamic but the
current build process isn't kind to that idea (in that the app is compiled
and run separately from the e2e tests, which aren't compiled at all).
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.

6 participants