Import multi-persona conformance toolkit from sandbox repo - #1
Import multi-persona conformance toolkit from sandbox repo#1nitparihar wants to merge 2 commits into
Conversation
Moves the WFM Supplier and Application Supplier conformance test toolkit (runner, data generator, Postman/Newman collections, test cases) from margo/sandbox conformance/ into this dedicated repo, flattened to the repo root. Source: margo/sandbox @ 7bbf36b (branch feature/multi-persona-conformance)
spulkit138
left a comment
spulkit138
left a comment
There was a problem hiding this comment.
As a general observation, code comments are missing, which makes the code unmaintainable in long run. Would advise to add code comments wherever necessary.
As a general rule, adding code comments for function headers, describing what they do & any pointers to keep in mind while using them; & adding comments where workflow is complex should suffice.
spulkit138
left a comment
spulkit138
left a comment
There was a problem hiding this comment.
Another observation, unit tests are not written. Writing unit test cases can help determine behaviour and reduce the amount of surprises or redundant testing-fixing effort cycle towards the end. It helps in maintaining codebase, and if someone changes some logic, unit test cases help in validating whether code changes are conformant to business logic or not.
Would advice to add unit test cases wherever required.
|
|
||
| switch v := value.(type) { | ||
|
|
||
| case []interface{}: |
There was a problem hiding this comment.
use of any instead of interface{} is more readable and compact.
It is being used almost everywhere, hence should be changed at those places, not commenting on every occurence.
| case []interface{}: | |
| case []any: |
| if !refMap[actual] { | ||
|
|
||
| fail( | ||
| report, | ||
| actual, | ||
| msg.Fail.Invalid, | ||
| ) | ||
|
|
||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a concept of "early exit" in Go, which can usually be implemented in any language but considered idiomatic in Go. It makes code readable & maintainable.
Following this approach also makes your code less "pecking to the right".
Please consider using this approach in Go code.
This is seen at multiple places, if you deem it right, should change at those places as well.
| if !refMap[actual] { | |
| fail( | |
| report, | |
| actual, | |
| msg.Fail.Invalid, | |
| ) | |
| return | |
| } | |
| } | |
| // This makes the loop to stop checking and continue to next | |
| if refMap[actual] { | |
| continue | |
| } | |
| fail( | |
| report, | |
| actual, | |
| msg.Fail.Invalid, | |
| ) | |
| return | |
| } |
| actual := formatActual( | ||
| item, | ||
| ) | ||
|
|
||
| if !refMap[actual] { | ||
|
|
||
| fail( | ||
| report, | ||
| actual, | ||
| msg.Fail.Invalid, | ||
| ) | ||
|
|
||
| return | ||
| } |
There was a problem hiding this comment.
This code chunk is seen multiple times, can a function be made with approapriate parameters which replaces this at multiple places? Code redundancy should be avoided. Please check for code redundancy at all other places as well.
|
|
||
| if strings.HasPrefix( | ||
| location, | ||
| "http://", |
There was a problem hiding this comment.
commonly used strings should be made a constant & then used elsewhere.
3 participants
Moves the WFM Supplier and Application Supplier conformance test toolkit (runner, data generator, Postman/Newman collections, test cases) from margo/sandbox conformance/ into this dedicated repo, flattened to the repo root.
Source: margo/sandbox @ 7bbf36b (branch feature/multi-persona-conformance)