Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add e2e test #151

Closed
wants to merge 38 commits into from
Closed

Conversation

gpBlockchain
Copy link
Collaborator

@gpBlockchain gpBlockchain commented Mar 27, 2023

What Changed

Resolve: #120

add e2e test

Motivation

Change Type

Indicate the type of change your pull request is:

  • documentation
  • patch
  • minor
  • major

Some pre-actions are required to trigger the E2E test

@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Merging #151 (d6408cc) into main (cd79532) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #151   +/-   ##
=======================================
  Coverage   86.75%   86.75%           
=======================================
  Files          41       41           
  Lines         770      770           
  Branches       95       95           
=======================================
  Hits          668      668           
  Misses         20       20           
  Partials       82       82           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@gpBlockchain gpBlockchain changed the title add e2e test test: add e2e test Mar 27, 2023
e2e/packages/e2e/__tests__/config/config.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,7 @@
import { setCommitHashToReport } from './util';

describe('get Env', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems it is not a test case, maybe a setup step for global.

Please use globalSetup

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At that time, I made some adjustments in globalSetup, but it seemed that the Allure plugin had not been loaded yet, which resulted in an error. Therefore, I directly wrote it in the test case instead.

e2e/packages/e2e/__tests__/popup/index.test.ts Outdated Show resolved Hide resolved
e2e/packages/e2e/__tests__/popup/index.test.ts Outdated Show resolved Hide resolved
e2e/packages/e2e/__tests__/popup/index.test.ts Outdated Show resolved Hide resolved
e2e/packages/e2e/src/nexus/page/wallet-manager-page.ts Outdated Show resolved Hide resolved
e2e/packages/e2e/src/nexus/servicer/preload.js Outdated Show resolved Hide resolved
e2e/packages/e2e/src/nexus/servicer/rpc.ts Outdated Show resolved Hide resolved
args: [`--disable-extensions-except=${option.nexusPath}`, `--load-extension=${option.nexusPath}`],
permissions: ['clipboard-read'],
slowMo: 10,
// // recordVideo: {
Copy link
Contributor

Choose a reason for hiding this comment

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

add a TODO or FIXME here for complain the reason.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the "recordVideo" function was too difficult to use, so I commented it out.now will remove it

{
files: '*.{ts,tsx}',
rules: {
'@typescript-eslint/no-explicit-any': 'error',
Copy link
Contributor

Choose a reason for hiding this comment

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

The better way is to modify these rules. I have seen many /* eslint-disable xxxxxxx */ in e2e. these rules may not suitable to enable on the e2e test files. @homura can we modify it? I need your opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are only 12 disable mark in this PR, and I found no disable mark for any, therefore, just keep it as it is

id: startWeb
if: success() || failure()
run: |
cd /home/runner/work/nexus/nexus/e2e/packages/nexus-web
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a ENV property for the path /home/runner/work/nexus/nexus?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This path is the fixed path in the action, need to modify it?

.github/workflows/e2e.yaml Outdated Show resolved Hide resolved
@homura
Copy link
Contributor

homura commented Mar 30, 2023

The e2e/packages/e2e's lock file seems to be missing

@gpBlockchain
Copy link
Collaborator Author

The e2e/packages/e2e's lock file seems to be missing

update package-lock.json

@homura
Copy link
Contributor

homura commented Mar 30, 2023

After the review, I think e2e needs to have some conventions, and here are some of my suggestions

Element Query

Priority

We can check out the testing-libarary

data-testid Standardization

use the data-testid to find an element, the data-testid recommend to use the BEM naming convention

block // 👍
my-block // 👍 dash to concatenate words
block__elem // 👍 element of a block
my-block__elem // 👍 dash block with element
my-block__another-elem // 👍 

MyBlock // 👎 migrate to dash 
myBlock // 👎 migrate to dash
block__elem__another-elem // 👎 multiple level element is not recommended

Testing Code

Follow the KISS (keep it simple, stupid) principle

  • one file, one scenario, e.g. import-wallet, create-wallet, ownership-sign-tx
  • only abstract if it is necessary, e.g. createTestEnv()
// ownership-sign-tx
// init with default mnemonic and whitelist
describe("fullOwnership sign transaction", () => {
  it('should send a signed tx successfully', () => {
    await createTestEnv({
      launchCkb: true
      // a ckb object implemented by playwright
    }, ({ rpc, ckb, nexus, browser }) => {
      // open the nexus-e2e
      await browser.open("http://localhost:3000")

      // simulate a dApp to assemble a transaction
      const provider = new FullOwnershipProvider(ckb)
      txSkeleton = await provider.injectCapacity({
        amount: CkbAmount.from(1000)
      })
      // NO await here, since we need do something on Nexus extension
      const signTxPromise = provider.signTransaction(tx)
      
      await nexus.focus();
      await nexus.fill(`[type="password"]`, DEFAULT_PASSWORD)
      await nexus.click({text: "Approve"})

      // await signed tx here
      const signature = await signTxPromise 
      const txHash = await sendTransaction(tx)
      await rpc.waitForNextBlock()
      expect(await rpc.getTransaction(txHash)).not.empty()
    })
  })
})

@homura homura mentioned this pull request Apr 3, 2023
4 tasks
@homura homura mentioned this pull request Apr 17, 2023
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.

Integrate nexus-e2e with Nexus GitHub workflow
4 participants