test: Bundle test state into single var #1645
Conversation
AndrewSisley
added
area/testing
Codecov ReportPatch coverage has no change and project coverage change:
@@ Coverage Diff @@
## develop #1645 +/- ##
===========================================
+ Coverage 75.62% 75.64% +0.02%
===========================================
Files 200 200
Lines 20807 20807
===========================================
+ Hits 15735 15739 +4
+ Misses 3994 3991 -3
+ Partials 1078 1077 -1
Flags with carried forward coverage won't be shown. Click here to find out more. see 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| case <-allActionsDone: | ||
| allActionsAreDone = true | ||
| } | ||
| for _, node := range getNodes(action.NodeID, s.nodes) { |
There was a problem hiding this comment.
question: It's not quite clear to me why we now have a for loop here. Could you give a quick explanation please?
There was a problem hiding this comment.
All of the other actions do this. A test case may involve many nodes, is just this one and a couple of other stragglers hadn't implemented it yet.
There was a problem hiding this comment.
But it still worked and didn't affect outcome?
There was a problem hiding this comment.
The NodeID property was added along with the loop here. Before it was essentially hardcoded to only ever act on the first node, regardless of the test configuration.
If you look at the commit it should be easier to understand I think.
There was a problem hiding this comment.
These test were never meant to have more than one node but it could be useful in the future 👍
| &result.GQL, | ||
| action, | ||
| ) | ||
| for _, node := range getNodes(action.NodeID, s.nodes) { |
There was a problem hiding this comment.
todo: Can you write a line here to document what the addition of this for loop enables?
There was a problem hiding this comment.
This loop is a copy-paste of the same line in the other 20 or so actions. I don't really think it needs to chew up anymore eyespace. It is not unusual.
It is a bit of a nuisance, and an inconsitency in the test suite. Note the diff is a bit mangled in some places due to an indentation change
It is good if these handlers are as self contained as possible.
It is good if action handlers are as self contained as possible
AndrewSisley
force-pushed
the
1644-test-ctx
branch
from
e2a7a22
to
85441b9
Compare
## Relevant issue(s) Resolves sourcenetwork#1644 ## Description Bundles all test state into single var. Stuff was starting to get too messy as the action system expanded. This makes all the stuff consistent, in name and location and removes the need to pass the individual items around as and when needed. All props have been documented. It also simplifies refreshing state, as return values are no longer needed. It opens the way to making the action system interface-based, which we can look at in the future if we want but not here in this PR.
Relevant issue(s)
Resolves #1644
Description
Bundles all test state into single var. Stuff was starting to get too messy as the action system expanded. This makes all the stuff consistent, in name and location and removes the need to pass the individual items around as and when needed. All props have been documented.
It also simplifies refreshing state, as return values are no longer needed. It opens the way to making the action system interface-based, which we can look at in the future if we want but not here in this PR.