-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Accounts - 4.x System E2E Tests #5285
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crypto.subtle is available in firefox 34 , there is error in tests because : "This feature is available only in secure contexts (HTTPS)" and CI logs shows tests are running without SSL
web3-eth-accounts: TypeError: crypto_1.crypto.web.subtle is undefined
350
web3-eth-accounts: getBrowserKey@http://localhost:43915/__cypress/tests?p=test/integration/account.test.ts:54252:18
Your Render PR Server URL is https://web3js-docs-pr-5285.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-cbjqk3vho1kvb2r30gtg. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Good work. Just need some minor changes.
templates/cypress/plugins/index.js
Outdated
@@ -27,6 +27,7 @@ module.exports = (on, config) => { | |||
config.env.WEB3_SYSTEM_TEST_MNEMONIC = process.env.WEB3_SYSTEM_TEST_MNEMONIC; | |||
config.env.WEB3_SYSTEM_TEST_PORT = process.env.WEB3_SYSTEM_TEST_PORT; | |||
config.env.WEB3_SYSTEM_TEST_PROVIDER = process.env.WEB3_SYSTEM_TEST_PROVIDER; | |||
config.env.WEB3_SYSTEM_TEST_CLIENT = process.env.WEB3_SYSTEM_TEST_CLIENT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should refer client
for eth node, in our repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
However, there are some minor points. Which I hope you modify them in this MR or create follow-up tasks for them.
beforeAll(async () => { | ||
contract = await contract.deploy(deployOptions).send(sendOptions); | ||
let contractDeployed: Contract<typeof ERC20TokenAbi>; | ||
beforeEach(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, do we really need here to have those repeated before each?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
let web3: Web3; | ||
|
||
beforeAll(async () => { | ||
beforeEach(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do all the lines need to be executed beforeEach
? I mean, It seems the following needed to be executed only once:
clientUrl = getSystemTestProvider();
web3 = new Web3(clientUrl);
await waitForOpenConnection(web3);
Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
// if (process.env.WEB3_SYSTEM_TEST_ENGINE === 'firefox') { | ||
// const port = parseInt(String(Math.random() * 10000 + 10000)); | ||
// config.clientCertificates = [ | ||
// { | ||
// url: 'https://web3.js', | ||
// certs: [ | ||
// { | ||
// cert: './cypress/.cert/cert.pem', | ||
// key: './cypress/.cert/key.pem', | ||
// }, | ||
// ], | ||
// }, | ||
// ]; | ||
// config.e2e.port = port; | ||
// config.e2e.hosts = { | ||
// 'web3.js': '127.0.0.1', | ||
// }; | ||
// config.e2e.baseUrl = `https://web3.js:${port}`; | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a comment that possibly refers to an issue that describes why this code is commented. And when this code is supposed to be uncommented.
Thanks,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
expect.objectContaining({ from: accounts[0] }), | ||
); | ||
}); | ||
itIf(!isIpc)( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, why this test should not apply to IPC
? And why it was working when it was applied to all including IPC?
And if there a good reason for that, could we please, have a small comment hereon why this test should not apply to IPC
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Description
#5038
Type of change
Checklist:
npm run dtslint
with success and extended the tests and types if necessary.npm run test:cov
and my test cases cover all the lines and branches of the added code.npm run build
with success.dist/web3.min.js
in a browser.CHANGELOG.md
file in the root folder.