Investigate end-to-end testing

valiafetisov commented 2 years ago

Goal

Outlines of different end-to-end testing possibilities

Context

We want to introduce some high-level testing scenarios where the main features of our main UI are tested. For example: that list of auctions is displayed and a user is able to participate in one of them (eg execute both auth steps and the transaction). The test should also independently check that blockchain have expected changes (eg Take event has been created)

I expect both should be possible:

a) on a highest-level, ie via headless browser-based testing tool that installs metamask, spawns hardhat simulation and "click" buttons to reach desired final state
b) via execution of specific (core) functions (or using a bot against hardhat)
c) same as a) but without a full browser and metamask, jest-style (metamask is replaced with setSigner call)

Note: for actual implementation this feature most probably would need #222

Tasks

[x] Investigate feasibility of both approaches
[x] Propose testing approach
[ ] Propose concrete testing tools/frameworks

KirillDogadin-std commented 2 years ago

Brief overview of tools that are right on the surface

for the hightest level there're quite young solutions such as

https://github.com/Synthetixio/synpress https://github.com/CraftAcademyLabs/cypress-metamask

that allow to integrate with metamask.

if we drop the metamask requirement, then the wrappers above are not needed and the core library can be used which is cypress

Questions:

1:

via execution of specific (core) functions

I assume that you're referring to the corresponding package and then wonder whether hardhat testing as described here is already sufficient to the (b) option or if it lacks some functionality.

2:

jest-style

if (1) question is resolved in the direction of the hardhat testing lacking the functionality, then this option becomes somehow the one that is covered by the testing approach mentioned, correct?

valiafetisov commented 2 years ago

I assume that you're referring to the corresponding package

Correct

and then wonder whether hardhat testing as described here is already sufficient to the (b) option or if it lacks some functionality.

The link describes different way to test, using different testing frameworks (waffle, chai) but also no frameworks at all (ie using UI as we're doing it in the moment). In general, for this particular issue, more end-to-end tests are preferable (eg a over c over b), but we of course have to know how much do we sacrifice for that (hacks, ci runtime). Both packages you've liked above looks promising

if (1) question is resolved in the direction of the hardhat testing lacking the functionality, then this option becomes somehow the one that is covered by the testing approach mentioned, correct?

c) is the same as a) but with a "hack" to avoid Metamask requirement. But since it's already seems feasible to use Metamask, we can ignore c) option atm.

KirillDogadin-std commented 2 years ago

Alright.

Then I assume that this https://github.com/CraftAcademyLabs/cypress-metamask is the option to first try out since it's most end-to-end, apparently requires less setup than the alternatives. We will have to see how it works out with metamask, for the rest it very much appears to be a solid wrapper around the established js library (ts supported)

valiafetisov commented 2 years ago

Can you please do a complete comparison between those two packages (or more) that you proposed initially? Ie: outline important qualities that we need to compare, then make a table with them:

	synpress	cypress-metamask
main difference	wrapper around Cypress with metamask	synpress reworked into a plugin
starts
maintainers
earliest commit
other
important
qualities
eg		requires less setup

And them conclude based on the results 🙂

I think we should also do the same with waffle vs chai vs etc? Or does those plugins from above require some specific test framework already?

KirillDogadin-std commented 2 years ago

	synpress	cypress-metamask	cypress with hack for metamask
main difference	wrapper around Cypress with metamask	synpress reworked into a plugin	have to tweak every test
starts	155	30	38 000
maintainers	13	2	353
latest commit	3 mar 22	9 apr 22	3 days ago
setup	harder	easier	ez
how hard (presumably) to write tests with	mild	mild-hard	hardest
simplicity	less straightforward	more stripped down	mock metamask is hard and then simple
documentation (feature set, gotchas, ...)	not-outlined	outlined on the frontpage	full-on-documentation website
examples on frontpage	present	not present	present +videos
headless mode (run without browser) support	unknown but presumably not present	not present	have to hack for metamask mocking anyway, so supported

as for waffle vs chai statement, i dont see how these are "vs" while reading https://hardhat.org/guides/waffle-testing.html . It seems it's more like these work together in one single testing setup.

on the other hand there're two sections that could potentially be compared:

[Testing with ethers.js & Waffle](https://hardhat.org/guides/waffle-testing.html)
[Testing with Web3.js & Truffle](https://hardhat.org/guides/truffle-testing.html)

which to me seem like quite different still: due to the fact that one of them (truffle) has to have the src code of the contract in the repositorty; while the other (waffle) apparently just mocks the behaviour of the contract instead of the direct execution of such. So since we're developing a ui and not SCs, i would give preference to the waffle setup

with the above, the option (b) in our case would then look like calling ui functions that display/calculate some values based on the contract outcome and mocking the contract returns. This is not really end-to-end, but rather a selective unit-testing though.

valiafetisov commented 2 years ago

Thanks for the beautiful table! So do you still want to start with cypress-metamask? (Seems reasonable to me)

I would assume they all support headless browser mode, since they are testing frameworks / plugins. Or do you mean something else by headless, ie run without browser? The metamask is the browser plugin, so running complete end-to-end test without a browser wouldn't be possible unless we're taking b or c route. And since some tricky functionality is attached to metamask (eg network switch), it would be beneficial to include it into tests.

KirillDogadin-std commented 2 years ago

TL;DR

lets start with cypress-metamask

cypress-metamask

i would go with it because:

still these 2 frameworks are the best shot apparently.
cypress-metamask is simplier to start with (apparetnly) and even if from the project-health (starts/maintainer number/patch frequency) it looks a bit worse, i still like it more because:
- if it's easy to start with - we can try it out and if it doesnt work out we just drop it. And the result comes relatively fast.

additional point to mention is that sypress has "real life examples" instead of documentation examples. https://github.com/Kwenta/kwenta is one of those (others are ref-d on the sypres project page). So if we adapt it we could perhaps follow the approach that is already outlined in the linked dApp repository.

This paragraph above does not neglect the chance that the same could be done with cypress-metamask since it's supposed to be the "same but simplier" package.

I would assume they all support headless browser mode,

from https://github.com/Synthetixio/synpress#-important

Tests work only in headed mode because extensions are not supported in headless mode

so we have to run headed apparently. That presumably hinders CI a lot.

tricky functionality is attached to metamask (eg network switch)

after this comment i would also like to mention that if it's too tricky, than we might prefer to reevaluate the plugins' functionality since

cypress-matamask has smaller (apparently) set of functions than sypress

valiafetisov commented 2 years ago

Tests work only in headed mode because extensions are not supported in headless mode
so we have to run headed apparently. That presumably hinders CI a lot.

Oh wow, would it require CI to have some sort of virtual display? I guess the answer is yes. This sounds very cumbersome. Then let's drop metamask requirement and switch to the c as the main route: we would still run it in browser, but would have to patch store and pre-execute setSigner before running the test.

KirillDogadin-std commented 2 years ago

sacrificing the extensions-support for the ease of CI makes sense, i agree.

For now, so that my vision is aligned with yours, could you please elaborate on

patch store

or give an example of the entity (func, object, ...) that has to be patched?

or was it a general phrase about the overall approach?

then i assume, if one digs the specifics, we would patch the store object in the test while exposing the store to cypress

and for setSigner i assume we preventively use getSigner with a pre-defined address to receive a fake-signer and a localhost chain id

valiafetisov commented 2 years ago

could you please elaborate on

patch store

Yes, the idea behind end-to-end test is to check whether user is able to execute specific transaction, but also to reach this state by clicking buttons. Eg:

Open https://unified-auctions.makerdao.com/
Click participate
Click on the first action
Navigate to the swap transaction (the button should be clickable, since sometimes it can be disabled)
Connect a wallet: here we would have to:
- execute setSigner like it's done inside bot https://github.com/sidestream-tech/unified-auctions-ui/blob/8d976c1c74618c3179059c311d232d2dcf2ee949/bot/src/keeper.ts#L20
- execute wallet/setAddress store action like it's done in the metamask (so the UI knows that wallet is connected) https://github.com/sidestream-tech/unified-auctions-ui/blob/8d976c1c74618c3179059c311d232d2dcf2ee949/frontend/lib/wallets/MetaMask.ts#L84
Click both auth buttons, one after another, wait till transaction has completed
Click execute, wait till transaction has completed
Check that transaction actually happened in the blockchain (eg wallet has DAI)

and for setSigner i assume we preventively use getSigner with a pre-defined address to receive a fake-signer and a localhost chain id

See the code of the bot linked above, the signer can be created from the private key. In case of the test, it will be one of the hardhat private keys, which already contain some ETH for transaction fees.

KirillDogadin-std commented 2 years ago

ok, approach makes sense. Since we're aligned on the (c) and the overall implementation idea is worded, we could close this issue and proceed further with the implementation issue. Should i create one?

valiafetisov commented 2 years ago

Would you be able to outline the complete testing stack here or do you need to make your hands dirty in order to do that?

In other words: What libraries will be used cypress/playwright/something else? Would those tests be part of the /frontend/tests folder or /tests? Would you need to run hardhat as a separate container or would you just add it as dev dependency to the frontend?

KirillDogadin-std commented 2 years ago

I would say that it requires the dirty hands :) - without the prior xp with the testing tool and the project development i'd for sure forget some important detail while estimating just from distance.

KirillDogadin-std commented 2 years ago

Additional insight has appeared while working on #282 , cypress apparently does not combine well with the number of requests we fire in order to fetch the auctions - for more see the post in the linked issue. And the number of the requests is (according what i've seen in the codebase) not really controllable / patchable / adjustable since it's severely influenced by the ethers.js.

After some further search for a ready-to-go solution, i failed to discover something that would solve the runtime problem caused by the number of requests to fire. I also would prefer to avoid mocking the state since then:

it stops being a truly e2e because the state should be defined by the external source that is "faked" by hardhat already.
would require to play around with the store (actions) and essentially alter the behaviour there - again seems not really e2e anymore. :e.g. disabling the update of auctions in favour of stubbed state.

It begins to seem like the better (from perspective of writing and maintaining tests) solution is to go with more conservative flow and do:

decouple the logic from the ui
test the core functionality (mb even store functionality) separately via e.g. jest. (essentially option (b))
have ui components in the storybook

KirillDogadin-std commented 2 years ago

regarding the proposal in https://github.com/sidestream-tech/unified-auctions-ui/issues/277#issuecomment-1141421561 :

is this supposed to work without stubbing the ethereum.js ? because the metamask wallet object is only intialized if the ethereum.isConnected is true (link). And this info is fetched from the window itself. If you instantly have a comment on this or know how ethereum can be mocked right away, could you point the direction?

valiafetisov commented 2 years ago

is this supposed to work without stubbing the ethereum.js

We're not using ethereum.js if there is such a thing. And ethers.js that we're using has nothing to do with the logic you've described: window.ethereum is injected by metamask extension.

As proposed in the initial comment: instead of trying to mock anything, including metamaks, we only need to:

execute setSigner in order for the core to have the signer and be able to sign transactions
execute wallet/setAddress action in order for the frontend application to believe that the wallet is connected (and know its address)

This, in theory, should make frontend application fully functional without the need for metamask extension or mocking.

KirillDogadin-std commented 2 years ago

okay, sadly the debugging session so far resulted in the conclusion that the signer gets "lost" along the way.

if i execute the process as follows

    let signerAddress;
    it('Clicks the button', function () {
        cy.visit(URL_TO_VISIT);
        cy.get('table').first({ timeout: 140000 }).should('contain.text', 'ETH');
        setSigner(ETHEREUM_NETWORK, createSigner(ETHEREUM_NETWORK, KEEPER_WALLET_PRIVATE_KEY));
        getSigner(ETHEREUM_NETWORK).then(signer =>
            signer.getAddress().then(addr => {
                signerAddress = addr;
                console.log(addr);
            })
        );
        cy.window()
            .its('$nuxt')
            .then(nuxt => nuxt.$store.dispatch('wallet/setAddress', signerAddress));
        cy.get('button').eq(7).should('contain.text', 'Participate').click();
    });

i see the ui with the mocked wallet "attached" and/but the singer "gets deleted" for some reason - proceeding with the ui flow to swap into profit, i could not authorize the transaction because the signers object has no keys inside and thus the error No signer has been created for the "localhost" network appears. Printing the singers on getSigners - shows that the object is empty.

Do you have any instant idea to why is that happening or should i go on with the debugging?

valiafetisov commented 2 years ago

js is an async language, so you can't solve the problem by using global variable

something.then(() => {
    // will run second
});
// will run first

Please always prefer async/await pattern instead of then for promises in our codebase, cypress seem to support it.

valiafetisov commented 2 years ago

Another question: how do you ensure that cy.get('button') is executed after cy.get('table') if you don't await it?

KirillDogadin-std commented 2 years ago

1

global variable

are you referring to the signerAddress?

cypress seem to support it.

yes, although with a warning that it is an antipattern :/

if thens are replaced with async/await the problem persists. I am not sure though that i correctly understood your problem description though :)

2

how do you ensure that cy.get('button') is executed after cy.get('table') if you don't await it?

that's a magic from cypress itself apparently, e.g. see example https://docs.cypress.io/guides/end-to-end-testing/writing-your-first-end-to-end-test#Step-3-Click-an-element or even better here https://docs.cypress.io/guides/end-to-end-testing/writing-your-first-end-to-end-test#Adding-more-commands-and-assertions .

valiafetisov commented 2 years ago

if thens are replaced with async/await the problem persists

Can you please link the actual code where the problem persists?

that's a magic from cypress itself apparently

Oh, maybe that's the same problem behind so load times? "magic" delays inserted by cypress. Should we maybe switch to plain jest with puppeteer until it's too late?

KirillDogadin-std commented 2 years ago

https://github.com/sidestream-tech/unified-auctions-ui/blob/bbbbc87d18397da1fe7949c90b7a3f67184f667d/frontend/cypress/integration/basic-spec.ts#L10

that's the requested link

i will try out the puppeteer and see if it's better :)

valiafetisov commented 2 years ago

I see the problem: since you run things within the browser, you have two isolated js contexts: one of the test itself and one of the browser. That's why, for example, you can't just use $nuxt.etc, but have to access it from the browser first. Therefore, setSigner should be executed within the context of the page as well. You can do it by simply creating a store method wallet/createWalletFromPrivateKey which will accept private key and do the rest and then call it from cypress.

KirillDogadin-std commented 2 years ago

yes, 🤦🏻 🤦🏻‍♂️ 🤦🏻‍♀️

best part is that i thought of this one but forgot to move the setAddress into the store function and labeled the approach as non-working.

scope question:

we dont initialise / mock the wallet
should i do it? should i expect the nuxt error when the "execute" button is hit?

valiafetisov commented 2 years ago

should i do it? should i expect the nuxt error when the "execute" button is hit?

No, why? The execution buttons should be fully functional if setSigner has executed and the address is set to the store. The scope of the first test is to validate that the all buttons are executed by validating that DAI balance of the current user has increased (this can be or maybe even should called outside of the browser). All other things may not be validated in this very first test (amount of auctions, numbers, etc)

KirillDogadin-std commented 2 years ago

No, why?

when i hit the execute button (final step), i see the nuxt error that the wallet is not initialized which is true because it was not.

The execution buttons should be fully functional if setSigner has executed

I don't see how `setSigner' adjusts the wallet object and when we hit execute, we need the wallet precreated which means that if we did not set the wallet (which i did not find), we dont proceed with the transaction.

The scope of the first test

then i assume i should also mock the wallet

valiafetisov commented 2 years ago

nuxt error that the wallet is not initialized

It always help to be more precise and paste the exact error message you receive. But I get the problem now. Yes, you can probably mock getWallet().address or better replace it with rootGetters['wallet/getAddress'] since it's duplicative

sidestream-tech / unified-auctions-ui