fix: Fix tests after qgb changes #133
Conversation
rach-id
changed the title
rach-id
changed the title
There was a problem hiding this comment.
these changes make sense! we should probably document whatever our decision on which approach we're taking in terms of either using this approach or the SetOrchestrator approach, along with the factors that went into that decision
I would approve with a patch for the failing test (either we find the patch that we made before or change the existing test to expect to fail) and some documentation of what decision we want to make.
|
|
sounds good |
There was a problem hiding this comment.
@evan-forbes if you're alright with PR, we can merge it.
No need to spend more time digging in history for the fix, I commented that test 😎
client/flags/flags.go
Outdated
| @@ -76,6 +76,11 @@ const ( | |||
| // Tendermint logging flags | |||
| FlagLogLevel = "log_level" | |||
| FlagLogFormat = "log_format" | |||
|
|
|||
| // QGB related flags | |||
| // FIXME: can we have these | |||
There was a problem hiding this comment.
It's alright to leave these here?
I checked also for the other flags, they are defined two times, here and under context also.
There was a problem hiding this comment.
they are defined two times, here and under context also
I would assume so. where are the second definitions?
There was a problem hiding this comment.
There was a problem hiding this comment.
@evan-forbes Let me know if this is alright so that I merge.
Thanks
There was a problem hiding this comment.
ahh I see, we actually probably want to remove those, as they are not global flags. Only the global flags for txs get loaded into that context. When we need access to those flags, we should access via parsing the flags in the definition of the cli command
There was a problem hiding this comment.
will remove them from client/context then. Thanks
| @@ -30,9 +31,6 @@ const ( | |||
| FlagGenesisFormat = "genesis-format" | |||
| FlagNodeID = "node-id" | |||
| FlagIP = "ip" | |||
|
|
|||
There was a problem hiding this comment.
now they are defined under general flags because they're also used in gentx, sim etc
|
ahh found it 434b308 we should add this before merging instead of commenting out imho, as it ensures that we don't forget about it. It probably won't matter, as we will likely upgrade soon, but out of good habit |
client/flags/flags.go
Outdated
| @@ -76,6 +76,11 @@ const ( | |||
| // Tendermint logging flags | |||
| FlagLogLevel = "log_level" | |||
| FlagLogFormat = "log_format" | |||
|
|
|||
| // QGB related flags | |||
| // FIXME: can we have these | |||
There was a problem hiding this comment.
they are defined two times, here and under context also
I would assume so. where are the second definitions?
…r struct (celestiaorg#133) * first pass on tests fixes * fixes the rest of unit tests * remove unnecessary comments * uses default eth address when starting sim network * cosmetics * comments failing test * comments failing test * uncomments test and fixes it from commit 434b308 * remove wrong testnet initialization
…r struct (celestiaorg#133) * first pass on tests fixes * fixes the rest of unit tests * remove unnecessary comments * uses default eth address when starting sim network * cosmetics * comments failing test * comments failing test * uncomments test and fixes it from commit 434b308 * remove wrong testnet initialization
…r struct (celestiaorg#133) * first pass on tests fixes * fixes the rest of unit tests * remove unnecessary comments * uses default eth address when starting sim network * cosmetics * comments failing test * comments failing test * uncomments test and fixes it from commit 434b308 * remove wrong testnet initialization
Attempts to fix the tests after the QGB related changed