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

integration tests: don't hardcode testnet constant values #2164

Conversation

arvidj
Copy link
Contributor

@arvidj arvidj commented Nov 29, 2022

Fixes #2163

This is quite a hacky solution, where the values are only checked if they match an actual RPC endpoint. This means that RPC endpoints have to be updated in two places when they change.

Another possible solution: include a key in the test configurations configuration.ts::Config like testnet: boolean or network_type: Testnet | Sandbox.

Release Note Draft Snippet

If relevant, please write a summary of your change that will be suitable for
inclusion in the Release Notes for the next Taquito release.

@netlify
Copy link

netlify bot commented Nov 29, 2022

Deploy Preview for taquito-test-dapp ready!

Name Link
🔨 Latest commit 046c852
🔍 Latest deploy log https://app.netlify.com/sites/taquito-test-dapp/deploys/6423e7f19f6053000735a0b8
😎 Deploy Preview https://deploy-preview-2164--taquito-test-dapp.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@arvidj arvidj marked this pull request as ready for review November 29, 2022 12:35
@roxaneletourneau
Copy link
Collaborator

I am not sure about the suggested changes, because the tests can be run using a different RPC URL. The second option (network_type: Testnet | Sandbox) might be more appropriate.

@arvidj arvidj force-pushed the arvid@#2163-dont-hardcode-rpc-constants branch from 82014df to 55e4b4d Compare December 6, 2022 15:37
@arvidj
Copy link
Contributor Author

arvidj commented Dec 6, 2022

I am not sure about the suggested changes, because the tests can be run using a different RPC URL. The second option (network_type: Testnet | Sandbox) might be more appropriate.

Hi, sorry for the slow update. I've implemented what you suggested above, but I had to do some refactoring to simplify the change. What do you think?

@arvidj arvidj force-pushed the arvid@#2163-dont-hardcode-rpc-constants branch from 55e4b4d to 918ac08 Compare December 7, 2022 09:19
@roxaneletourneau
Copy link
Collaborator

Thank you @arvidj !
Nice cleanup in the config.ts file.
The changes look good to me.
I replaced jakartanet with limanet (because we merged our lima branch yesterday to master).

await setup(true);

writeOutput("import { KnownContracts } from './known-contracts';")
appendOutput("export const knownContracts" + protocolShort + "KnownContracts = {");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think KnownContracts should not be part of the string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a new version, which should fix this. The intention was to concatenate the type annotation, i.e. + ": KnownContracts" but the colon got lost. Should be fixed now.

@arvidj arvidj force-pushed the arvid@#2163-dont-hardcode-rpc-constants branch 3 times, most recently from a36de2c to 1f0dd60 Compare December 14, 2022 18:19
const limanetEphemeral: Config =
defaultConfig({
networkName: 'LIMANET',
protocol: Protocols.PtJakart2,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrong protocol hash -> should be lima, not jakarta

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the latest version.

@arvidj arvidj force-pushed the arvid@#2163-dont-hardcode-rpc-constants branch from 1f0dd60 to aa300d9 Compare December 14, 2022 20:42
@arvidj
Copy link
Contributor Author

arvidj commented Dec 15, 2022

I've lost a commit in the rebase, hang on.

@roxaneletourneau
Copy link
Collaborator

There are failures in the flextesa jobs for the "originate-known-contracts" step:
image
I think these outputs are used by the "mondaynet" job, but this could be simplified by using the addresses from "https://github.com/ecadlabs/taquito/blob/master/integration-tests/known-contracts-ProtoALph.ts". Any thoughts about it @danielelisi?

const operation = await tezos.contract.originate(contractOriginateParams);
const contract = await operation.contract();
console.log(`known ${contractName} address: ${contract.address}`);
console.log(`::set-output ${contractName}::${contract.address}\n`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This ::set-output command needs a name parameter to fix the issue mentioned in #2164 (comment)

@danielelisi
Copy link
Contributor

There are failures in the flextesa jobs for the "originate-known-contracts" step: image I think these outputs are used by the "mondaynet" job, but this could be simplified by using the addresses from "https://github.com/ecadlabs/taquito/blob/master/integration-tests/known-contracts-ProtoALph.ts". Any thoughts about it @danielelisi?

Yes either way will work. We need to dynamically originate and pass these contract addresses to the Mondaynet integration test job because the network is reset every week.

@roxaneletourneau
Copy link
Collaborator

There are failures in the flextesa jobs for the "originate-known-contracts" step: image I think these outputs are used by the "mondaynet" job, but this could be simplified by using the addresses from "https://github.com/ecadlabs/taquito/blob/master/integration-tests/known-contracts-ProtoALph.ts". Any thoughts about it @danielelisi?

Yes either way will work. We need to dynamically originate and pass these contract addresses to the Mondaynet integration test job because the network is reset every week.

Yes, we have two ways of doing the same thing, maybe we should only keep one of them.

@arvidj arvidj force-pushed the arvid@#2163-dont-hardcode-rpc-constants branch from ba33521 to dcd2a23 Compare December 19, 2022 13:18
@arvidj
Copy link
Contributor Author

arvidj commented Dec 19, 2022

There are failures in the flextesa jobs for the "originate-known-contracts" step: image I think these outputs are used by the "mondaynet" job, but this could be simplified by using the addresses from "https://github.com/ecadlabs/taquito/blob/master/integration-tests/known-contracts-ProtoALph.ts". Any thoughts about it @danielelisi?

Yes either way will work. We need to dynamically originate and pass these contract addresses to the Mondaynet integration test job because the network is reset every week.

Yes, we have two ways of doing the same thing, maybe we should only keep one of them.

In the latest version, I've restored the format of the ::set-output command. I now get:

> [email protected] test:originate-known-contracts
> node -r ts-node/register originate-known-contracts.ts

known contract address:  KT1WNPeBPfBjSALHBdhYgUhUAAfPDXvbAYwC
::set-output name=knownContractAddress::KT1WNPeBPfBjSALHBdhYgUhUAAfPDXvbAYwC

known bigMapContract address:  KT19bvsjNpN4KcGB5SfpSvpj8jBcCyfQH5Fm
::set-output name=knownBigMapContractAddress::KT19bvsjNpN4KcGB5SfpSvpj8jBcCyfQH5Fm

known tzip12BigMapOffChainContract address:  KT1CefqoEa95ga6SDWCUfxn2p3rVPmVJ3t1e
::set-output name=knownTzip12BigMapOffChainContractAddress::KT1CefqoEa95ga6SDWCUfxn2p3rVPmVJ3t1e

known saplingContract address:  KT1GzWFKdw7txm1fnjRuMCx913s7V8fLJ1xR
::set-output name=knownSaplingContractAddress::KT1GzWFKdw7txm1fnjRuMCx913s7V8fLJ1xR

known onChainViewContractAddress address:  KT1THQyTne7npn7vyw9WXw4oSbXpoJDY5ZHf
::set-output name=knownOnChainViewContractAddressAddress::KT1THQyTne7npn7vyw9WXw4oSbXpoJDY5ZHf

txRollupAddress:  txr1NFYQM8rkbKX9sqA4MkB6N9ifKp5zzMNZC

################################################################################

I can't validate it further though. Guess we'll have to run the flextesa tests?

I also noticed that ::set-output is deprecated: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

It doesn't seem to hard adopting to the new workflow. I could have originate-known-contracts.ts output a JSON file instead, and then read that in config.ts respectively .github/workflows/integration-tests-mondaynet.ts.

Preferably in a follow-up PR though :)

@arvidj arvidj force-pushed the arvid@#2163-dont-hardcode-rpc-constants branch from dcd2a23 to 66f5b28 Compare December 19, 2022 13:26
@arvidj arvidj force-pushed the arvid@#2163-dont-hardcode-rpc-constants branch from 66f5b28 to ae30caa Compare March 6, 2023 13:24
@arvidj arvidj force-pushed the arvid@#2163-dont-hardcode-rpc-constants branch from ae30caa to 76f3d85 Compare March 14, 2023 07:48
@arvidj arvidj force-pushed the arvid@#2163-dont-hardcode-rpc-constants branch from 76f3d85 to 046c852 Compare March 29, 2023 07:25
@zainen
Copy link
Contributor

zainen commented Apr 19, 2023

Hi @arvidj,
Just wanted to check in to see if its ok if i take over the rest of managing the pr and rebasing it. I would have to interact with your forked repo to bring it up to date for the pr

alternatively could you rebase it and we can merge into another branch that can be finalized

@arvidj
Copy link
Contributor Author

arvidj commented Apr 25, 2023

Hi @arvidj, Just wanted to check in to see if its ok if i take over the rest of managing the pr and rebasing it. I would have to interact with your forked repo to bring it up to date for the pr

alternatively could you rebase it and we can merge into another branch that can be finalized

Sure, just tell me what I need to do. Sorry for the slow response.

@zainen zainen changed the base branch from master to arvidj-integration-tests-constants May 15, 2023 17:05
@zainen
Copy link
Contributor

zainen commented May 15, 2023

Hi @arvidj,
Sorry for the delay! Could you rebase your pr? Then we will merge it into this branch, and then I can finalize it from here and merge it! That way, I won't need to work on your repo 😄

@arvidj arvidj force-pushed the arvid@#2163-dont-hardcode-rpc-constants branch from 046c852 to 27befe5 Compare July 17, 2023 12:22
@arvidj
Copy link
Contributor Author

arvidj commented Jul 17, 2023

Hello @arvidj if you have a moment please rebase so @zainen can merge it and finalize :)

Hi @arvidj,
Sorry for the delay! Could you rebase your pr? Then we will merge it into this branch, and then I can finalize it from here and merge it! That way, I won't need to work on your repo smile

Hi, sorry for the delay. I just pushed a rebased version.

@arvidj arvidj force-pushed the arvid@#2163-dont-hardcode-rpc-constants branch from 27befe5 to 47d4beb Compare August 30, 2023 07:53
@arvidj
Copy link
Contributor Author

arvidj commented Aug 30, 2023

@dsawali I just rebased, could you have a look?

@dsawali dsawali merged commit 29c2eff into ecadlabs:arvidj-integration-tests-constants Aug 30, 2023
dsawali added a commit that referenced this pull request Aug 30, 2023
)

* integration-tests: refactor knownContracts configuration

* integration-tests: refactor config.ts

* integration-tests: add [NetworkType] configuration parameter

This enables the distinction between 'real' test networks, such as
Jakartanet, and locally spawned sandbox networks.

* integration-tests: only run [rpc-get-protocol-constants] in testnet

Co-authored-by: arvidj <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants