Skip to content

Remove obsoleted integration tests#1447

Merged
elenadimitrova merged 3 commits intoexperimentalfrom
remove-integration-tests
Sep 21, 2021
Merged

Remove obsoleted integration tests#1447
elenadimitrova merged 3 commits intoexperimentalfrom
remove-integration-tests

Conversation

@elenadimitrova
Copy link
Copy Markdown
Contributor

@elenadimitrova elenadimitrova commented Sep 13, 2021

Remove read-proxy-event.spec.ts and erc20.spec.ts tests as well as some obsoleted integration tests contracts.

Closes ENG-1327

@elenadimitrova elenadimitrova self-assigned this Sep 13, 2021
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Sep 13, 2021

⚠️ No Changeset found

Latest commit: cc8b7e8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added 2-reviewers A-op-batcher Area: op-batcher A-ops Area: ops labels Sep 13, 2021
@elenadimitrova elenadimitrova changed the base branch from develop to experimental September 13, 2021 11:48
@elenadimitrova elenadimitrova marked this pull request as draft September 13, 2021 11:49
@elenadimitrova elenadimitrova force-pushed the remove-integration-tests branch from a79e794 to 5a5cc34 Compare September 15, 2021 10:22
@github-actions github-actions bot added 2-reviewers and removed A-geth A-op-batcher Area: op-batcher A-ops Area: ops labels Sep 15, 2021
@elenadimitrova elenadimitrova marked this pull request as ready for review September 15, 2021 10:31
@K-Ho
Copy link
Copy Markdown
Contributor

K-Ho commented Sep 15, 2021

Rather than removing, I'd rather just replace the single line of
npx hardhat --config ./hardhat.config.js test:integration:l2 --compile --deploy
with
npx hardhat --config ./hardhat.config.js test:integration:dual --compile --deploy

#1261

Copy link
Copy Markdown
Contributor

@smartcontracts smartcontracts left a comment

Choose a reason for hiding this comment

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

LGTM. I think it's fine to remove all of this.

@smartcontracts
Copy link
Copy Markdown
Contributor

smartcontracts commented Sep 15, 2021

Errr ok I see the point about the dual SNX tests. Not 100% sure how I feel about it. @elenadimitrova I'll defer to you, if you feel like it would be useful to keep the SNX L1<>L2 tests then feel free to keep it.

@elenadimitrova
Copy link
Copy Markdown
Contributor Author

elenadimitrova commented Sep 15, 2021

I tried the test:integration:dual run here https://github.com/ethereum-optimism/optimism/runs/3614189833?check_suite_focus=true and that errors with

Error: fee too low: 2350710000000, use at least tx.gasLimit = 1626714 and tx.gasPrice = 15000000

@K-Ho is it because they haven’t updated the gasPrice in their config to be non-zero and set to 15000000?

EDIT: Also tried running against the branch here Synthetixio/synthetix#1504 but this time the error was

gas required exceeds allowance (11000000)

EDIT: The branch above now returns a different error

------ IMPORT FEE PERIODS ------

freshDeploy - no fee periods required for import. Skipping.

------ CONFIGURE STANDLONE FEEDS ------

Attempting action: ExchangeRates.addAggregator(SNX,0xe044814c9eD1e6442Af956a817c161192cBaE98F)
An unexpected error occurred:

Error: cannot estimate gas; transaction may fail or may require manual gas limit (error={"reason":"processing response error","code":"SERVER_ERROR","body":"{\"jsonrpc\":\"2.0\",\"id\":3554,\"error\":{\"code\":-32603,\"message\":\"Error: Transaction reverted without a reason string\"}}","error":{"code":-32603},"requestBody":"{\"method\":\"eth_estimateGas\",\"params\":[{\"type\":\"0x2\",\"maxFeePerGas\":\"0x9502f900\",\"maxPriorityFeePerGas\":\"0x3b9aca00\",\"from\":\"0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266\",\"to\":\"0x9fe653933300a05bf60d19901031da8008653a6e\",\"data\":\"0x3f0e084f534e580000000000000000000000000000000000000000000000000000000000000000000000000000000000e044814c9ed1e6442af956a817c161192cbae98f\"}],\"id\":3554,\"jsonrpc\":\"2.0\"}","requestMethod":"POST","url":"http://localhost:9545"}, tx={"data":"0x3f0e084f534e580000000000000000000000000000000000000000000000000000000000000000000000000000000000e044814c9ed1e6442af956a817c161192cbae98f","to":{},"maxPriorityFeePerGas":{"type":"BigNumber","hex":"0x3b9aca00"},"from":"0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266","type":2,"maxFeePerGas":{"type":"BigNumber","hex":"0x9502f900"},"nonce":{},"gasLimit":{},"chainId":{}}, code=UNPREDICTABLE_GAS_LIMIT, version=abstract-signer/5.4.1)
    at Logger.makeError (/Users/Elena/Source/optimism/integration-tests/synthetix/node_modules/@ethersproject/logger/src.ts/index.ts:225:28)
    at Logger.throwError (/Users/Elena/Source/optimism/integration-tests/synthetix/node_modules/@ethersproject/logger/src.ts/index.ts:237:20)
    at /Users/Elena/Source/optimism/integration-tests/synthetix/node_modules/@ethersproject/abstract-signer/src.ts/index.ts:301:31
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async Promise.all (index 7) {
  reason: 'cannot estimate gas; transaction may fail or may require manual gas limit',
  code: 'UNPREDICTABLE_GAS_LIMIT',

@elenadimitrova elenadimitrova force-pushed the remove-integration-tests branch from 6f4ee3a to 6266c34 Compare September 19, 2021 12:17
@github-actions github-actions bot added the A-integration Area: integration tests label Sep 19, 2021
@smartcontracts
Copy link
Copy Markdown
Contributor

I vote we just skip the SNX tests and figure out a new ticket for running external tests + figure out exactly which external tests we want to run

@elenadimitrova elenadimitrova force-pushed the remove-integration-tests branch from 6266c34 to cc8b7e8 Compare September 21, 2021 09:58
@elenadimitrova elenadimitrova merged this pull request into experimental Sep 21, 2021
@elenadimitrova elenadimitrova deleted the remove-integration-tests branch September 21, 2021 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-integration Area: integration tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants