Add Drizzle tests #5467
Add Drizzle tests #5467
Conversation
|
I think this has merit. I think that we shouldn't have this in the same repository though as this will incur a large maintenance overhead. I noticed that in #5450 Dan linked to his issue-5425-repro repository—I think it might be best if we kept these as separate repositories and somehow introduced a generalized way of testing external dapps against each MM build. That way we could allow users to submit dapps as test cases. That way we could (one day) test against the top N dapps. Thoughts? 🤔 |
|
@whymarrh I understand your point and I think it’s fair. I should have added some emphasis that the main goal of this PR is not to test drizzle itself but to make sure we don’t break compatibility with web3 1.0 (which should be part of our test suite after the provider subs branch gets merged into dev). We used drizzle because it’s probably the most popular framework using it, and yesterday while debugging the subs issue we agreed that it was necessary to add it to the test suite.
EDIT : added truffle unbox drizzle so we always get the latest version of it and it doesn't need any extra maintenance other than make sure the tests still pass Pinging @danfinlay @kumavis @thuang to hear their opinions. |
brunobar79
force-pushed
the
drizzle-tests
branch
from
eb6fe73 to
111998e
Compare
|
I think you both have good points:
Part of me agrees with Whymarrh, thinking maybe we could just periodically run "the larger test suite" before a release, but that just postpones fixing new bugs. I tend to think this is kindof exactly what the test suite is for, and as long as we run it in parallel, it should be fine to add, right? |
|
I think this is a fine addition, as my main (only?) concern was the bloat included from |
|
@whymarrh done |
| this.bail(true) | ||
|
|
||
| before(async function () { | ||
| switch (process.env.SELENIUM_BROWSER) { |
There was a problem hiding this comment.
This selenium bootstrapping should be extracted into a utility (I'm sure it already is somewhere). You shouldn't need more than a line or two for this generic setup
There was a problem hiding this comment.
Sounds good! l’ll handle tomorrow.
| @@ -6,5 +6,5 @@ set -o pipefail | |||
|
|
|||
| export PATH="$PATH:./node_modules/.bin" | |||
|
|
|||
| shell-parallel -s 'npm run ganache:start -- -b 2' -x 'sleep 5 && static-server test/e2e/beta/contract-test/ --port 8080' -x 'sleep 5 && mocha test/e2e/beta/metamask-beta-ui.spec' | |||
| shell-parallel -s 'npm run ganache:start -- -d -b 2' -x 'sleep 5 && static-server test/e2e/beta/contract-test/ --port 8080' -x 'sleep 5 && mocha test/e2e/beta/from-import-beta-ui.spec' | |||
There was a problem hiding this comment.
Because there’s nothing on that spec that requires the static server
| }) | ||
|
|
||
|
|
||
| describe('New UI setup', async function () { |
There was a problem hiding this comment.
All this generic init tests should also be abstracted into a library
|
Agree with @whymarrh about making it a separate PR |
|
@whymarrh @kumavis Just created a new issue (#5488) to discuss with @tmashuang and @danjm what would be the best approach for this and will fix in a separate PR. |
* added drizzle app for testing * working * clean up * clean up script * make build step required * add drizzle-tests to .eslintignore * clean up drizzle run script * lint * use truffle unbox * undo eslintignore changes * revert change * dont use global * dont need this steps * use the new account flow * restore package-lock.json
This PR adds a drizzle app to our test suite as the initial step towards making sure we don't break web3 1.0 / drizzle support in the future.
For now the only test we have is just to check that we're exposing the address correctly but ideally we should check other basic stuff like send eth / tokens and ideally subs. Since all the workflow is already in place it should be pretty easy to add additional scenarios.