test(node): Cover textual repository nodes #8512
Conversation
Signed-off-by: Frank Viernau <[email protected]>
fviernau
force-pushed
the
npm-handle-textual-repository-nodes
branch
from
e6dba7e to
cb39f8c
Compare
| @@ -196,7 +195,7 @@ class NpmSupportTest : WordSpec({ | |||
| @Suppress("Wrapping") | |||
| val node = ObjectMapper().run { | |||
| createObjectNode().apply { | |||
| replace("gitHead", TextNode(HashAlgorithm.SHA1GIT.emptyValue)) | |||
| replace("gitHead", TextNode("bar")) | |||
There was a problem hiding this comment.
Commit message: This is not about the "VCS path", but the "VCS revision".
| @@ -73,3 +73,5 @@ val EMPTY_JSON_NODE: JsonNode = MissingNode.getInstance() | |||
| inline fun <reified T> String.fromYaml(): T = yamlMapper.readValue(this) | |||
|
|
|||
| fun Any?.toYaml(): String = yamlMapper.writeValueAsString(this) | |||
|
|
|||
| fun String.readJsonTree(): JsonNode = jsonMapper.readTree(this) | |||
There was a problem hiding this comment.
Commit message: s/programatical/programatic/
| replace("url", TextNode("https://example.com/")) | ||
| replace("directory", TextNode("foo")) | ||
| }) | ||
| val node = """ |
There was a problem hiding this comment.
It'd be some work, but I guess ideally this should be done for all tests in this file now, or? That would nicely decouple the test from Jackson as well.
There was a problem hiding this comment.
I would do a follow up PR for this
| ) | ||
| } else { | ||
| val type = repository["type"].textValueOrEmpty() | ||
| val url = repository.textValue() ?: repository["url"].textValueOrEmpty() |
There was a problem hiding this comment.
I believe the original repository.textValue() here already handles the repository.isTextual case, or?
At least I'm pretty sure that the original code already intended to fully handle that case:
typewould be empty, soVcsType.forName(type)returnsVcsType.UNKNOWNpathwould be empty as well
So what additional case does your new code cover?
There was a problem hiding this comment.
HM, are you sure that the previous code already worked in this case?
When I wrote this code (in my other PR) I do re-call that the analyzer previously did not detect VCS info but afterwards did. So, thought it was needed but probably I made a mistake during testing and had made further changes which made it work.
There was a problem hiding this comment.
Test proofs that you were right.
Do not use the constant for the empty value as this can be misleading one into thinking there may be special handling for that. So, use an arbitrary string as for the VCS revision. Signed-off-by: Frank Viernau <[email protected]>
Use a JSON string instead of programatic construction of the JSON object. Signed-off-by: Frank Viernau <[email protected]>
The repository nodes can be textual, see [1]. Add a test to clarify the parsing works for that. [1]: https://docs.npmjs.com/cli/v10/configuring-npm/package-json#repository Signed-off-by: Frank Viernau <[email protected]>
fviernau
force-pushed
the
npm-handle-textual-repository-nodes
branch
from
cb39f8c to
b4aa154
Compare
fviernau
changed the title
See individual commits.