-
Notifications
You must be signed in to change notification settings - Fork 217
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
Testing pool fetching runtime selection #2225
Conversation
f020bc3
to
439f8f0
Compare
@@ -303,7 +303,13 @@ x-settings: &settings | |||
properties: | |||
pool_metadata_source: | |||
<<: *poolMetadataSource | |||
description: How to fetch pool metadata | |||
description: | | |||
Select stake pool metadata fetching strategy: |
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.
The values are already explained here, no?
x-poolMetadataSource: &poolMetadataSource
description: |
Pool metadata source. This sets the metadata fetching strategy.
Possible values are
* null -> no fetching
* direct -> direct fetching
* uri -> use SMASH server
oneOf:
- type: string
enum:
- none
- direct
- title: URI
type: string
format: uri
example: https://smash.cardano-mainnet.iohk.io/
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.
Yes, but I figured it renders nicely in the API doc. :) (see screenshot in Comment in the pr)
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.
Hmm, how do we make sure the doc doesn't go async with the other place?
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.
There's this -> https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs#L411-L414.
Descriptions are manual maintenance, but having stuff clearly explained improves user experience.
- `direct` - metadata is fetched directly from chain, | ||
- `uri` - metadata is fetched from the external Stake-Pool Metadata Aggregation Server (SMASH) | ||
|
||
After update existing metadata will be dropped forcing it to re-sync automatically with the new setting. |
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.
This sentence is useful indeed!
eventually "1. There is no metadata" $ do | ||
metadatas <- getMetadata | ||
metadatas `shouldSatisfy` (all isNothing) |
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.
Haven't checked if it compiles, but a little golfing so we could have it on one line maybe:
eventually "1. There is no metadata" $ do | |
metadatas <- getMetadata | |
metadatas `shouldSatisfy` (all isNothing) | |
eventually "1. There is no metadata" $ getMetadata >>= (`shouldSatisfy` all isNothing) |
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.
@@ -735,6 +737,7 @@ spec = describe "SHELLEY_STAKE_POOLS" $ do | |||
saturation `shouldSatisfy` (any (> 0)) | |||
|
|||
it "contains pool metadata" $ \ctx -> do | |||
updateMetadataSource ctx "direct" |
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.
Hmm, what's the default of the tests? Why did it work before? 😄
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.
It used to check on-chain by default before, but now its none
... that's what it is also now in the integration test cluster.
Yes, I was surprised it worked, with this check passing:
metadataActual
`shouldSatisfy` (`Set.isSubsetOf` metadataPossible)
so it turns out that:
> (fromList []) `isSubsetOf` (fromList [1])
True
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.
Lolo
it "SETTINGS_02 - Changing pool_metadata_source re-syncs metadata" $ \ctx -> do | ||
let toNone = "none" | ||
toDirect = "direct" | ||
getMetadata = map (view #metadata) . snd <$> unsafeRequest |
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.
I personally prefer fmap
over map
, because even if we change from list to set or nonempty list, it still compiles... but ymmv
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.
@@ -771,6 +774,8 @@ spec = describe "SHELLEY_STAKE_POOLS" $ do | |||
mapMaybe (fmap getApiT . view #metadata) pools | |||
metadataActual | |||
`shouldSatisfy` (`Set.isSubsetOf` metadataPossible) | |||
metadataActual | |||
`shouldSatisfy` (not . Set.null) |
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.
Therefore I added this...
1d13fd5
to
c109bd8
Compare
bors r+ |
2225: Testing pool fetching runtime selection r=piotr-iohk a=piotr-iohk # Issue Number #2163 / [ADP-427](https://jira.iohk.io/browse/ADP-427) # Overview - 8c0c580 More detailed description of poolMetadataSource in swagger.yaml - 439f8f0 Integration test checking metadata is re-synced after settings update - 1d13fd5 Fix existing stake pool test checking metadata exists # Comments ![Screenshot from 2020-10-08 14-06-59](https://user-images.githubusercontent.com/42900201/95456553-ca995b80-096f-11eb-89ee-b1b42ae8d40f.png) Co-authored-by: Piotr Stachyra <[email protected]>
Timed out.
#expected |
bors r+ |
2225: Testing pool fetching runtime selection r=piotr-iohk a=piotr-iohk # Issue Number #2163 / [ADP-427](https://jira.iohk.io/browse/ADP-427) # Overview - 8c0c580 More detailed description of poolMetadataSource in swagger.yaml - 439f8f0 Integration test checking metadata is re-synced after settings update - 1d13fd5 Fix existing stake pool test checking metadata exists # Comments ![Screenshot from 2020-10-08 14-06-59](https://user-images.githubusercontent.com/42900201/95456553-ca995b80-096f-11eb-89ee-b1b42ae8d40f.png) Co-authored-by: Piotr Stachyra <[email protected]>
Build failed:
#expected |
The setting here is less related to epoch or the chain. The resync can take an arbitrary amount of time, depending on the amount of pools metadata that needs fetching. So we probably want a custom timeout? |
I'm guessing the failure above is expected? I'm marking it so, but please change it to a reference to a flaky test ticket if that's the reason. |
Well, in integration tests there are only 3 pools, so I'd expect it to resolve rather fast 🤔 ... Anyway, I've changed timeout for those tests to 120s. I've actually also risen global timeout for |
bors try |
tryBuild failed:
#expected |
1e42823
to
a84c137
Compare
Ok, so it failed on hydra again with bigger timeout of 120s. I thought that maybe that's MacOS related, cause hydra is on Mac, but checked on MacOS VM (manually) and it works just fine. Changes are applied instantaneously on updating settings and metadata is fetched again (or dropped when setting to I added additional check to verify if metadata setting actually changed after updating... 🤷♂️ |
bors try |
Does the integration test run with an actual SQlite db or inside memory? |
Actual SQlite. |
tryBuild failed:
Same thing... #expected |
bors try |
tryBuild failed:
Ah, different one... |
bors try |
tryBuild failed:
|
bors try |
tryBuild succeeded: |
bors try |
tryBuild succeeded: |
bors r+ |
2225: Testing pool fetching runtime selection r=piotr-iohk a=piotr-iohk # Issue Number #2163 / [ADP-427](https://jira.iohk.io/browse/ADP-427) # Overview - 8c0c580 More detailed description of poolMetadataSource in swagger.yaml - 439f8f0 Integration test checking metadata is re-synced after settings update - 1d13fd5 Fix existing stake pool test checking metadata exists # Comments ![Screenshot from 2020-10-08 14-06-59](https://user-images.githubusercontent.com/42900201/95456553-ca995b80-096f-11eb-89ee-b1b42ae8d40f.png) Co-authored-by: Piotr Stachyra <[email protected]>
Build failed:
Seems like a And it seems stuck here:
|
… timeout for eventually to 100s
8d82bc7
to
f38e3a6
Compare
bors try |
tryBuild succeeded: |
bors r+ |
Build succeeded: |
Issue Number
#2163 / ADP-427
Overview
8c0c580
More detailed description of poolMetadataSource in swagger.yaml
439f8f0
Integration test checking metadata is re-synced after settings update
1d13fd5
Fix existing stake pool test checking metadata exists
Comments