Migration tests clean up #1739

piotr-iohk · 2020-06-10T09:11:04Z

Issue Number

Overview

78a6a1c
Extract Migrations for byron to separate scenario file
64a0416
Extract Migration tests for byron in core-integration
96bd585
Migration tests for Shelley
90853c3
Remove pending from byron migration tests
5e2f26c
Make SHELLEY_MIGRATE_01_big_wallet test pending due to issue Cannot migrate big wallet #1751
5c25b01
- Add ByronWallets tests to Shelley integration suite.
- Make Todo comment which tests to enable when Byron addresses/transactions are supported on the cardano-node.
- temporarily move BYRON_RESTORE special cases involving restoring 'funky' ledger and icarus wallets to Transactions so we can run ByronWallet tests on Shelley integration suite

Comments

Attempting to "clean-up" migration tests and add migration tests for Shelley and Byron.

~~I have added some byron (random + icarus) wallets into genesis.yaml, but... It doesn't really work, node crashes... 🤔~~ ... because they are not supported yet.

KtorZ · 2020-06-10T12:16:02Z

lib/shelley/test/data/cardano-node-shelley/genesis.yaml

+  82d818582183581ca02e0d8a3c4bede18c1cf8229ddb02769c03ac0dd3776b8c34f17cc8a0001af6579671: 100000000000
+  82d818582183581c963f621368a253a75fc950c25493216ef6543e7dbe5dbe652a8eb978a0001a8b5d87da: 100000000000
+  82d818582183581c812f3071c5c9f9a7ea95d6e98f910173f912097a6a75351322c1c867a0001afd55c87f: 100000000000
+


I believe that these would need to be base58-encoded as being "byron addresses".
Can you also double-check with the node's guys whether Byron addresses as initial genesis funds are supported yet ?

Currently when you create a byron wallet (e.g. on FF) the address is:
For random:
82d818584683581cd7c21027dd1037acc53b9ce171f7f909ba68972b56886c00e7122392a201581e581c427a66d6279d26c4538f97f8df0ce0d98ce5de06cbf45a7ede422b1b0242182a001ac4109f66

For icarus:
82d818582583581c4b3a47fcc6ebc7018955105eaf4fa37ca215d084370f299b3be00f4ea10242182a001afe67739c

Anyway Byron transactions are not supported yet, I learned, so I guess, it's still too early for that...

I have removed Byron addresses from genesis for now.

piotr-iohk · 2020-06-11T13:39:34Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Migrations.hs

+              testAddressCycling 10
+
+    it "SHELLEY_MIGRATE_01 - \
+        \ migrate a big wallet requiring more than one tx" $ \ctx -> do


This test is failing with:

Failures: src/Test/Integration/Scenario/API/Shelley/Migrations.hs:221:15: 1) API Specifications SHELLEY_MIGRATE_01 - migrate a big wallet requiring more than one tx expected: Status {statusCode = 202, statusMessage = "Accepted"} but got: Status {statusCode = 500, statusMessage = "Internal Server Error"} from the following response: Left (DecodeFailure "{\"code\":\"created_invalid_transaction\", \"message\":\"That's embarrassing. It looks like I've created an invalid transaction that could not be parsed by the node. Here's an error message that may help with debugging: ApplyTxError [LedgerFailure (UtxowFailure (UtxoFailure (MaxTxSizeUTxO 20614 4096)))]\"}")

Yes. The max number of inputs is currently hard-coded to 100 in the Shelley transaction layer. We need something a bit better here :)

Shall I make this test pending and create a ticket for that perhaps?

piotr-iohk · 2020-06-11T14:54:15Z

bors try

iohk-bors · 2020-06-11T15:30:37Z

try

Build failed

buildkite/cardano-wallet

KtorZ · 2020-06-11T15:43:05Z

lib/shelley/test/integration/Main.hs

-                HWWalletsCLI.spec @n
-                PortCLI.spec @t
-                NetworkCLI.spec @t
+        describe "API Specifications" $ specWithServer tr $ do


Hey! No!

There's a wrong merge-conflict resolution here I think ^.^ .. I moved the specWithServer above to be around ALL, otherwise, this actually restarts a cluster between both steps. Not only does it takes time, but it also doubles the number of genesis credentials we need for starting stake pools. The Migrations.spec @n can be added all the same.

oh ok, whatever it was... it was done automatically (I think) 😅 I'll fix.

KtorZ · 2020-06-11T15:45:15Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Migrations.hs

+    , PaymentAddress n ByronKey
+    ) => SpecWith (Context t)
+spec = do
+    it "SHELLEY_CALCULATE_01 - \


Do you think we could mimic the "new clean way of writing scenarios" that we have in recently written integration spec files? (i.e., declare scenario as separate function and reference them in the main "spec")

To be honest I don't find that new way handy for me. I recognize this is cleaner/more aesthetic maybe:

describe "BYRON_MIGRATE" $ do scenario_MIGRATE_01 @n fixtureRandomWallet scenario_MIGRATE_02 @n fixtureRandomWallet 1 scenario_MIGRATE_02 @n fixtureRandomWallet 3 scenario_MIGRATE_02 @n fixtureRandomWallet 10 scenario_MIGRATE_02 @n fixtureIcarusWallet 1 scenario_MIGRATE_02 @n fixtureIcarusWallet 3 scenario_MIGRATE_02 @n fixtureIcarusWallet 10

But you have to scroll down/scroll up to see what particular test is doing. This is ok(ish) when there is small number of scenarios in a file, but when there are many of them it becomes painful (to me).

Ok, maybe that's my personal preference (or I just got used to it so much), but I find good ol spec -> describe -> it scheme (+ some helper methods for repeatable actions) a good compromise between "clean" and sufficient level of detail in the (integration) test suite such that I can immediately tell what is going on...

KtorZ · 2020-06-12T13:44:59Z

lib/shelley/test/integration/Main.hs

@@ -139,6 +141,8 @@ main = withUtf8Encoding $ withLogging Nothing Info $ \(_, tr) -> do
        specWithServer tr $ do
            describe "API Specifications" $ do
                Addresses.spec @n
+                ByronMigrations.spec @n


I think it's a bit too soon to enable this one on the shelley node.

They have been enabled (those tests were part of Wallets.spec, but they were made "pending"), but since they are now extracted to separate spec, then maybe they just don't have to be enabled for shelley indeed.

I have removed those tests from Shelley suite and also added ByronWallet.spec. In 5c25b01.

piotr-iohk · 2020-06-12T15:20:26Z

bors try

iohk-bors · 2020-06-12T15:29:35Z

try

Build failed

buildkite/cardano-wallet

…omment` which tests to enable when Byron addresses/transactions are supported on the cardano-node. - temporarily move BYRON_RESTORE special cases involving restoring 'funky' ledger and icarus wallets to Transactions so we can run ByronWallet tests on Shelley integration suite

piotr-iohk · 2020-06-12T15:34:33Z

bors try

iohk-bors · 2020-06-12T16:14:18Z

try

Build succeeded

piotr-iohk · 2020-06-12T16:24:27Z

bors r+

iohk-bors · 2020-06-12T16:51:58Z

Build succeeded

piotr-iohk added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label Jun 10, 2020

piotr-iohk self-assigned this Jun 10, 2020

KtorZ reviewed Jun 10, 2020

View reviewed changes

piotr-iohk force-pushed the piotr/migration-tests-clean-up branch from e8869c3 to ae23d11 Compare June 11, 2020 13:32

piotr-iohk commented Jun 11, 2020

View reviewed changes

piotr-iohk force-pushed the piotr/migration-tests-clean-up branch from ae23d11 to cc8a7c9 Compare June 11, 2020 14:15

iohk-bors bot added a commit that referenced this pull request Jun 11, 2020

Try #1739:

d2e58b6

KtorZ reviewed Jun 11, 2020

View reviewed changes

piotr-iohk force-pushed the piotr/migration-tests-clean-up branch from cc8a7c9 to d6d8dec Compare June 12, 2020 11:20

KtorZ reviewed Jun 12, 2020

View reviewed changes

KtorZ approved these changes Jun 12, 2020

View reviewed changes

piotr-iohk mentioned this pull request Jun 12, 2020

Cannot migrate big wallet #1751

Closed

Piotr Stachyra added 4 commits June 12, 2020 17:16

Extract Migrations for byron to separate scenario file

78a6a1c

Extract Migration tests for byron in core-integration

64a0416

Migration tests for Shelley

96bd585

Remove pending from byron migration tests

90853c3

piotr-iohk force-pushed the piotr/migration-tests-clean-up branch from d6d8dec to c9dac8e Compare June 12, 2020 15:17

iohk-bors bot added a commit that referenced this pull request Jun 12, 2020

Try #1739:

0b02032

Piotr Stachyra added 2 commits June 12, 2020 17:32

Make SHELLEY_MIGRATE_01_big_wallet test pending due to issue #1751

5e2f26c

piotr-iohk force-pushed the piotr/migration-tests-clean-up branch from c9dac8e to 5c25b01 Compare June 12, 2020 15:32

iohk-bors bot added a commit that referenced this pull request Jun 12, 2020

Try #1739:

89908cf

iohk-bors bot merged commit 63c485d into master Jun 12, 2020

iohk-bors bot deleted the piotr/migration-tests-clean-up branch June 12, 2020 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migration tests clean up #1739

Migration tests clean up #1739

piotr-iohk commented Jun 10, 2020 •

edited

Loading

KtorZ Jun 10, 2020

piotr-iohk Jun 10, 2020 •

edited

Loading

piotr-iohk Jun 11, 2020

piotr-iohk Jun 11, 2020

KtorZ Jun 11, 2020

piotr-iohk Jun 12, 2020

piotr-iohk commented Jun 11, 2020

iohk-bors bot commented Jun 11, 2020

KtorZ Jun 11, 2020

piotr-iohk Jun 12, 2020

KtorZ Jun 11, 2020

piotr-iohk Jun 12, 2020 •

edited

Loading

KtorZ Jun 12, 2020

piotr-iohk Jun 12, 2020

piotr-iohk Jun 12, 2020 •

edited

Loading

piotr-iohk commented Jun 12, 2020

iohk-bors bot commented Jun 12, 2020

piotr-iohk commented Jun 12, 2020

iohk-bors bot commented Jun 12, 2020

piotr-iohk commented Jun 12, 2020

iohk-bors bot commented Jun 12, 2020

Migration tests clean up #1739

Migration tests clean up #1739

Conversation

piotr-iohk commented Jun 10, 2020 • edited Loading

Issue Number

Overview

Comments

Choose a reason for hiding this comment

piotr-iohk Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-iohk commented Jun 11, 2020

iohk-bors bot commented Jun 11, 2020

try

Build failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-iohk Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-iohk Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

piotr-iohk commented Jun 12, 2020

iohk-bors bot commented Jun 12, 2020

try

Build failed

piotr-iohk commented Jun 12, 2020

iohk-bors bot commented Jun 12, 2020

try

Build succeeded

piotr-iohk commented Jun 12, 2020

iohk-bors bot commented Jun 12, 2020

Build succeeded

piotr-iohk commented Jun 10, 2020 •

edited

Loading

piotr-iohk Jun 10, 2020 •

edited

Loading

piotr-iohk Jun 12, 2020 •

edited

Loading

piotr-iohk Jun 12, 2020 •

edited

Loading