[release/1.3.1] [CO-354] Split test-suites for NMNothing/NMJust #3559

mhuesch · 2018-09-07T06:17:04Z

Description

"Split" the test-suites, for both NMMustBeNothing and NMMustBeJust cases. Also, introduce arbitrary ProtocolMagics (replace dummyProtocolMagic values). Since we hardcoded values in (3), both splits should pass.

Motivation: in order to follow best practices of modifying tests & source code in separate PRs, today I factored out the testsuite modifications into a separate PR. We are PR'ing this before the full implementation of NetworkMagic is PR'ed, because we want to demonstrate that the tests can be split & parameterized by arbitrary ProtocolMagics, without modifying source code, and still pass. Then, when we modify the source and pass the tests, we will have more confidence in a correct implementation.

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CO-354

Type of change

[~] 🐞 Bug fix (non-breaking change which fixes an issue)
🛠 New feature (non-breaking change which adds functionality)
[~] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
[~] 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
🔨 New or improved tests for existing code
[~] ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
[~] CHANGELOG entry has been added and is linked to the correct PR on GitHub.
^ there is no 1.3.1 section in the CHANGELOG, and this is dev work on a release branch, so I'm skipping this.

Testing checklist

I have added tests to cover my changes.
All new and existing tests passed.

mhuesch · 2018-09-07T06:19:17Z

N.B. this PR contains commits which are part of PR #3556 and PR #3558. Thus, the "Files Changed" is inflated, and if someone reviews this before #3556 and #3558 are merged, it would be simplest to review only the commits:

db3f23ae9 [CO-354] Fix `wallet-new` "example1" test
91db28b92 [CO-354] Split test-suites for NMNothing/NMJust

EDIT:
I, @intricate, have rebased this branch on release/1.3.1 after the merging of #3556 and #3558 so this comment no longer applies.

mhuesch · 2018-09-07T06:24:34Z

wallet-new/test/unit/Test/Infrastructure/Generator.hs

+    = round (calculateTxSizeLinear linearFeePolicy (estimateSize addrAttrSize 16 ins outs))
+  where
+    addrAttrSize = 128 + (case rnm of NMMustBeNothing -> 0
+                                      NMMustBeJust    -> 4)


@edsko @matt-noonan : would be great to get your feedback on this.

Since a NetworkMagic seems to take up 4 bytes, I add 4 to the AddrAttributes estimated byte size. I'm aware that there's now a better way to estimate things on develop, but this seems like a simple fix to me, unless I'm missing something.

I think it's fine, we have better checks on the 1.4 branch where this is anyway more important, so we can revisit this there.

The hardcoded 4 bytes has been updated to 7 bytes, since an Int32 has 3 more bytes than a Word8.

wallet-new/test/unit/WalletUnitTest.hs

mhuesch · 2018-09-12T07:41:39Z

I've just updated this PR to run the testsuites multiple times, with different ProtocolMagic values. Ideally, we would generate ProtocolMagic values inside of the Quickcheck prop. However, that would require significant rearchitecting of the testsuites. We plan to make the proper changes on develop. Over here on release/1.3.1, we will simply run the testsuites multiple times with several ProtocolMagic values generated via runIO in the Hspec Spec monad. See here for an example.

erikd · 2018-09-12T08:10:45Z

client/test/Test/Pos/Client/Txp/UtilSpec.hs

+
+-- We run the tests this number of times, with different `ProtocolMagics`, to get increased
+-- coverage. We should really do this inside of the `prop`, but it is difficult to do that
+-- without significant rewriting of the testsuite.


erikd · 2018-09-12T08:13:49Z

core/test/Test/Pos/Core/AddressSpec.hs

+    NMNothing -> 0
+    -- Encoding size:
+    -- Map key: 2 bytes (1 for header, 1 for Word8)
+    -- Map val: 1-5 bytes (1 for header, 0-4 for Word32)


Is this an Int32 now? Stupid compiler doesn't know how to type check comments :)

Ugh, fixed 😆

Ah that one's my fault. oops.

This implements part 3.5 of the ProtocolMagic' master plan. Merged with: [CO-354] Incorporate Luke's fix for `generator` testsuite

This test was failing, apparently due to the increased sizes of `AddrAttributes` in the NMJust case. I added 4 bytes to the maximum byte limit, which is the amount by which `AddrAttributes` grows when the `NetworkMagic` value is added.

intricate · 2018-09-12T12:57:36Z

crypto/test/Test/Pos/Crypto/Arbitrary.hs

@@ -54,6 +55,12 @@ instance Arbitrary ProtocolMagicId where
 instance Arbitrary RequiresNetworkMagic where
    arbitrary = elements [NMMustBeNothing, NMMustBeJust]

+genProtocolMagicUniformWithRNM :: RequiresNetworkMagic -> Gen ProtocolMagic


explorer/test/Test/Pos/Explorer/Socket/MethodsSpec.hs

intricate · 2018-09-12T13:01:32Z

generator/src/Test/Pos/Block/Logic/Mode.hs

@@ -102,7 +103,6 @@ import           Pos.WorkMode (EmptyMempoolExt)
 import           Test.Pos.Block.Logic.Emulation (Emulation (..), runEmulation, sudoLiftIO)
 import           Test.Pos.Configuration (defaultTestBlockVersionData, defaultTestConf,
                                         defaultTestGenesisSpec)
-import           Test.Pos.Crypto.Dummy (dummyProtocolMagic)


intricate · 2018-09-12T13:07:09Z

generator/test/Test/Pos/Binary/CommunicationSpec.hs

+            specBody pm
+
+specBody :: ProtocolMagic -> Spec
+specBody pm = withProvidedMagicConfig pm $ withCompileInfo def $


Replacing withStaticConfigurations with withProvidedMagicConfig feels a bit reminiscent of the issue we originally encountered in Test.Pos.Generator.Block.LrcSpec in which withProvidedMagicConfig makes use of withGenesisSpec while withStaticConfigurations doesn't. Since we're unsure as to whether the tests rely on this fact, we should probably stick with withStaticConfigurations.

intricate · 2018-09-12T13:08:10Z

generator/test/Test/Pos/Block/Logic/VarSpec.hs

+            specBody pm
+
+specBody :: ProtocolMagic -> Spec
+specBody pm = withProvidedMagicConfig pm $


Replacing withStaticConfigurations with withProvidedMagicConfig feels a bit reminiscent of the issue we originally encountered in Test.Pos.Generator.Block.LrcSpec in which withProvidedMagicConfig makes use of withGenesisSpec while withStaticConfigurations doesn't. Since we're unsure as to whether the tests rely on this fact, we should probably stick with withStaticConfigurations.

wallet/test/Test/Pos/Wallet/Web/Methods/BackupDefaultAddressesSpec.hs

To minimize risk of inadequate test coverage, due to too few `ProtocolMagic`s being used, we run the tests a specified number of times, with different `ProtocolMagic` values. We also use `choose` from Quickcheck to get uniformly distributed random values for the `ProtocolMagicId` field. `arbitrary`, by comparison, generates small Int32s, which is not desirable for testing because we want to exercise multiple sizes of CBOR encoding.

erikd · 2018-09-12T22:43:21Z

client/test/Test/Pos/Client/Txp/UtilSpec.hs

+    modifyMaxSuccess (`div` testMultiple) $ do
+        pm <- runIO (generate (genProtocolMagicUniformWithRNM rnm))
+        describe ("(requiresNetworkMagic=" ++ show rnm ++ ")") $
+            specBody pm


mhuesch requested review from adinapoli-iohk, akegalj, denisshevchenko, erikd, ksaric, parsonsmatt and vincenthz as code owners September 7, 2018 06:17

mhuesch requested review from matt-noonan and removed request for vincenthz, akegalj, denisshevchenko and ksaric September 7, 2018 06:17

mhuesch self-assigned this Sep 7, 2018

mhuesch commented Sep 7, 2018

View reviewed changes

mhuesch mentioned this pull request Sep 7, 2018

[release/1.3.1] [CO-354] Implement NetworkMagic logic #3561

Merged

6 tasks

mhuesch commented Sep 7, 2018

View reviewed changes

wallet-new/test/unit/WalletUnitTest.hs Show resolved Hide resolved

mhuesch added the DO NOT MERGE label Sep 7, 2018

mhuesch force-pushed the intricate+mhuesch/CO-354/split-testsuites branch from db3f23a to 864bf74 Compare September 10, 2018 19:01

mhuesch changed the title ~~[CO-354] Split test-suites for NMNothing/NMJust~~ [release/1.3.1] [CO-354] Split test-suites for NMNothing/NMJust Sep 10, 2018

intricate force-pushed the intricate+mhuesch/CO-354/split-testsuites branch 4 times, most recently from cc87ef4 to 864bf74 Compare September 11, 2018 16:18

mhuesch force-pushed the intricate+mhuesch/CO-354/split-testsuites branch from 864bf74 to bbe8266 Compare September 12, 2018 07:38

mhuesch removed the DO NOT MERGE label Sep 12, 2018

mhuesch force-pushed the intricate+mhuesch/CO-354/split-testsuites branch from bbe8266 to 54a1394 Compare September 12, 2018 08:00

erikd reviewed Sep 12, 2018

View reviewed changes

mhuesch added 2 commits September 12, 2018 01:17

[CO-354] Split test-suites for NMNothing/NMJust

dbc8b6d

This implements part 3.5 of the ProtocolMagic' master plan. Merged with: [CO-354] Incorporate Luke's fix for `generator` testsuite

[CO-354] Fix wallet-new "example1" test

d4a9ca6

This test was failing, apparently due to the increased sizes of `AddrAttributes` in the NMJust case. I added 4 bytes to the maximum byte limit, which is the amount by which `AddrAttributes` grows when the `NetworkMagic` value is added.

mhuesch force-pushed the intricate+mhuesch/CO-354/split-testsuites branch from 54a1394 to 63149f1 Compare September 12, 2018 08:18

intricate requested a review from ruhatch September 12, 2018 12:53

intricate suggested changes Sep 12, 2018

View reviewed changes

intricate previously approved these changes Sep 12, 2018

View reviewed changes

intricate dismissed their stale review via 547e635 September 12, 2018 17:58

intricate force-pushed the intricate+mhuesch/CO-354/split-testsuites branch from 1b09905 to 547e635 Compare September 12, 2018 17:58

Michael Hueschen and others added 2 commits September 12, 2018 13:08

[CO-354] Use withStaticConfigurations in CommunicationSpec and VarSpec

0b6404e

intricate force-pushed the intricate+mhuesch/CO-354/split-testsuites branch from 547e635 to 0b6404e Compare September 12, 2018 18:09

erikd reviewed Sep 12, 2018

View reviewed changes

erikd approved these changes Sep 12, 2018

View reviewed changes

parsonsmatt approved these changes Sep 13, 2018

View reviewed changes

jmitchell merged commit 3ea91b9 into release/1.3.1 Sep 13, 2018

intricate deleted the intricate+mhuesch/CO-354/split-testsuites branch September 13, 2018 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release/1.3.1] [CO-354] Split test-suites for NMNothing/NMJust #3559

[release/1.3.1] [CO-354] Split test-suites for NMNothing/NMJust #3559

mhuesch commented Sep 7, 2018

mhuesch commented Sep 7, 2018 •

edited by intricate

Loading

mhuesch Sep 7, 2018

edsko Sep 7, 2018

mhuesch Sep 12, 2018

mhuesch commented Sep 12, 2018

erikd Sep 12, 2018

erikd Sep 12, 2018

mhuesch Sep 12, 2018

intricate Sep 12, 2018

intricate Sep 12, 2018

intricate Sep 12, 2018

intricate Sep 12, 2018 •

edited

Loading

intricate Sep 12, 2018 •

edited

Loading

erikd Sep 12, 2018

[release/1.3.1] [CO-354] Split test-suites for NMNothing/NMJust #3559

[release/1.3.1] [CO-354] Split test-suites for NMNothing/NMJust #3559

Conversation

mhuesch commented Sep 7, 2018

Description

Linked issue

Type of change

Developer checklist

Testing checklist

mhuesch commented Sep 7, 2018 • edited by intricate Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhuesch commented Sep 12, 2018

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

Choose a reason for hiding this comment

intricate Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

intricate Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhuesch commented Sep 7, 2018 •

edited by intricate

Loading

intricate Sep 12, 2018 •

edited

Loading

intricate Sep 12, 2018 •

edited

Loading