464 lines
16 KiB
Plaintext
464 lines
16 KiB
Plaintext
{
|
|
"comments": [
|
|
{
|
|
"key": {
|
|
"uuid": "944d27b7_18050b68",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 17,
|
|
"author": {
|
|
"id": 16515
|
|
},
|
|
"writtenOn": "2021-01-20T07:52:57Z",
|
|
"side": 1,
|
|
"message": "import is not ordered correctly, and import should be like:\nfrom validations_libs import constants",
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": true
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "88cae3ad_3c45c825",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 17,
|
|
"author": {
|
|
"id": 32926
|
|
},
|
|
"writtenOn": "2021-01-20T09:06:26Z",
|
|
"side": 1,
|
|
"message": "I need only a single constant. \nGetting it with additional qualification (prepending constants), is possible but unnecessary. \nWe use the same structure of import on lines 25-27 for importing Runner. Ansible and fakes. \nIf it\u0027s a style recommendation I would change it on these lines too.\n\nAccording to pep8 both forms are acceptable. https://www.python.org/dev/peps/pep-0008/#imports",
|
|
"parentUuid": "944d27b7_18050b68",
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": false
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "47788195_8a1c40bd",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 17,
|
|
"author": {
|
|
"id": 16515
|
|
},
|
|
"writtenOn": "2021-01-20T09:36:22Z",
|
|
"side": 1,
|
|
"message": "But here, you are making an import for a variable only, please, change the form according to my comment and order it by name, meaning under line 26",
|
|
"parentUuid": "88cae3ad_3c45c825",
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": false
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "78627c2a_eaec987d",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 17,
|
|
"author": {
|
|
"id": 32926
|
|
},
|
|
"writtenOn": "2021-01-20T11:18:09Z",
|
|
"side": 1,
|
|
"message": "Done in next patch.",
|
|
"parentUuid": "47788195_8a1c40bd",
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": false
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "99f4d31b_65253ce0",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 19,
|
|
"author": {
|
|
"id": 16515
|
|
},
|
|
"writtenOn": "2021-01-20T07:52:57Z",
|
|
"side": 1,
|
|
"message": "I don\u0027t think it\u0027s a good idea to change this import in this patch.\nThere is no real benefit to this and it add a lot of useless and unrelated changes in the review.",
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": true
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "17abd417_80549632",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 19,
|
|
"author": {
|
|
"id": 32926
|
|
},
|
|
"writtenOn": "2021-01-20T09:06:26Z",
|
|
"side": 1,
|
|
"message": "Sadly the linter limits me to standard 80 cols. This way I can shave 4 on each decorator, giving me more space for code and avoiding indentation for sake of a single arg. \nSince, out of the entire mock package, we only use patch, it doesn\u0027t give us any benefit to import entire package instead of just one. \n\nWhile unrelated to creation of tests itself, it does simplify work to some degree. \nSince this patch, if merged, will already double (or more) number of lines this file has, and since I already altered several existing tests in this very file, I didn\u0027t see much need to worry about extra changes. \n\nAfter all, half of those decorators are on newly created test methods.",
|
|
"parentUuid": "99f4d31b_65253ce0",
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": false
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "732816c0_53c10a80",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 19,
|
|
"author": {
|
|
"id": 16515
|
|
},
|
|
"writtenOn": "2021-01-20T09:36:22Z",
|
|
"side": 1,
|
|
"message": "Well, this is not a valid reason for changing the whole syntax, you should handle the 80 col.\nThe previous statement that you changed was handling the 80 col, so please revert this change.",
|
|
"parentUuid": "17abd417_80549632",
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": false
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "c03f7eed_a20b6130",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 19,
|
|
"author": {
|
|
"id": 32926
|
|
},
|
|
"writtenOn": "2021-01-20T11:18:09Z",
|
|
"side": 1,
|
|
"message": "Done in next patch.",
|
|
"parentUuid": "732816c0_53c10a80",
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": false
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "69d1254f_fe4c7d8e",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 327,
|
|
"author": {
|
|
"id": 16515
|
|
},
|
|
"writtenOn": "2021-01-20T07:52:57Z",
|
|
"side": 1,
|
|
"message": "please use key: value instead of this, it\u0027s more readable and understandable.",
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": true
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "30eb16e0_8cfa3773",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 327,
|
|
"author": {
|
|
"id": 32926
|
|
},
|
|
"writtenOn": "2021-01-20T09:06:26Z",
|
|
"side": 1,
|
|
"message": "Yeah, I\u0027ll do that. Not sure why I had them as a tuple in a first place 😄.",
|
|
"parentUuid": "69d1254f_fe4c7d8e",
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": false
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "240ca7a5_17f752d2",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 381,
|
|
"author": {
|
|
"id": 16515
|
|
},
|
|
"writtenOn": "2021-01-20T07:52:57Z",
|
|
"side": 1,
|
|
"message": "same here, key: value",
|
|
"range": {
|
|
"startLine": 368,
|
|
"startChar": 0,
|
|
"endLine": 381,
|
|
"endChar": 9
|
|
},
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": true
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "faf9ff2f_feb4380d",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 381,
|
|
"author": {
|
|
"id": 32926
|
|
},
|
|
"writtenOn": "2021-01-20T09:06:26Z",
|
|
"side": 1,
|
|
"message": "Ack",
|
|
"parentUuid": "240ca7a5_17f752d2",
|
|
"range": {
|
|
"startLine": 368,
|
|
"startChar": 0,
|
|
"endLine": 381,
|
|
"endChar": 9
|
|
},
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": false
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "9b3ef29d_f1c839a2",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 422,
|
|
"author": {
|
|
"id": 16515
|
|
},
|
|
"writtenOn": "2021-01-20T07:52:57Z",
|
|
"side": 1,
|
|
"message": "same here.",
|
|
"range": {
|
|
"startLine": 409,
|
|
"startChar": 0,
|
|
"endLine": 422,
|
|
"endChar": 9
|
|
},
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": true
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "ef9f9a50_2652dcad",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 422,
|
|
"author": {
|
|
"id": 32926
|
|
},
|
|
"writtenOn": "2021-01-20T09:06:26Z",
|
|
"side": 1,
|
|
"message": "Ack",
|
|
"parentUuid": "9b3ef29d_f1c839a2",
|
|
"range": {
|
|
"startLine": 409,
|
|
"startChar": 0,
|
|
"endLine": 422,
|
|
"endChar": 9
|
|
},
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": false
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "dfabad23_8552c9fe",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 455,
|
|
"author": {
|
|
"id": 16515
|
|
},
|
|
"writtenOn": "2021-01-20T07:52:57Z",
|
|
"side": 1,
|
|
"message": "why not call assert_called_once_with() from mock_config instead.\nWhere is raised the FileNotFoundError in the code ?",
|
|
"range": {
|
|
"startLine": 451,
|
|
"startChar": 8,
|
|
"endLine": 455,
|
|
"endChar": 76
|
|
},
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": true
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "600de0c1_e1bd9242",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 455,
|
|
"author": {
|
|
"id": 32926
|
|
},
|
|
"writtenOn": "2021-01-20T09:06:26Z",
|
|
"side": 1,
|
|
"message": "The point is to ensure that the run method works with path correctly, and doesn\u0027t mangle it.\n\nThe asserts work in following fashion:\n1. We verify that open was called only once, and exactly once. \n Otherwise, our assumptions about how the method works are broken, \n and rest of the test won\u0027t work.\n2. If the path got mangled, passed incorrectly or is otherwise invalid,\n the run method has no way to recover, or to even notice. Until the path is used. \n Then it would fail, potentially far from the place where the issue started. \n The lstat function raises FileNotFoundError if, \n and only if, the path points to nonexistent target, in an existing directory.\n Since the path we use points to nonexistent targe, but inside existing dir (/tmp),\n we get FileNotFoundError. \n That way we verify that path is in valid format for our filesystem.\n3. Finally we check that the path we want was actually passed to the open function.\n If we didn\u0027t first check that the open function was called once,\n we would have multiple call_args. Checking all of them, while verifying the order of calls,\n would be even more confusing. This way we only have a single call. \n Using, as already discussed, valid path. All we need to do is check if the strings match.",
|
|
"parentUuid": "dfabad23_8552c9fe",
|
|
"range": {
|
|
"startLine": 451,
|
|
"startChar": 8,
|
|
"endLine": 455,
|
|
"endChar": 76
|
|
},
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": false
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "b0182c54_255c181d",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 455,
|
|
"author": {
|
|
"id": 16515
|
|
},
|
|
"writtenOn": "2021-01-20T09:36:22Z",
|
|
"side": 1,
|
|
"message": "iiuc you are trying to test part of the code that is handled by ansible-runner.\nIf you want to test the statement line 414 where ansible.py is creating ansible.cfg, then you should modify the code itself to catch exception if something wrong.",
|
|
"parentUuid": "600de0c1_e1bd9242",
|
|
"range": {
|
|
"startLine": 451,
|
|
"startChar": 8,
|
|
"endLine": 455,
|
|
"endChar": 76
|
|
},
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": false
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "d4a0f1f7_e1382c77",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 455,
|
|
"author": {
|
|
"id": 32926
|
|
},
|
|
"writtenOn": "2021-01-20T11:18:09Z",
|
|
"side": 1,
|
|
"message": "Remade the code to test the path actually passes to RunneConfig.",
|
|
"parentUuid": "b0182c54_255c181d",
|
|
"range": {
|
|
"startLine": 451,
|
|
"startChar": 8,
|
|
"endLine": 455,
|
|
"endChar": 76
|
|
},
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": false
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "6ff0fe9a_23b88f66",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 485,
|
|
"author": {
|
|
"id": 16515
|
|
},
|
|
"writtenOn": "2021-01-20T07:52:57Z",
|
|
"side": 1,
|
|
"message": "same here",
|
|
"range": {
|
|
"startLine": 481,
|
|
"startChar": 0,
|
|
"endLine": 485,
|
|
"endChar": 66
|
|
},
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": true
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "fb90312e_6652b690",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 485,
|
|
"author": {
|
|
"id": 32926
|
|
},
|
|
"writtenOn": "2021-01-20T09:06:26Z",
|
|
"side": 1,
|
|
"message": "as above...",
|
|
"parentUuid": "6ff0fe9a_23b88f66",
|
|
"range": {
|
|
"startLine": 481,
|
|
"startChar": 0,
|
|
"endLine": 485,
|
|
"endChar": 66
|
|
},
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": false
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "7aa023d9_2dde2911",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 513,
|
|
"author": {
|
|
"id": 16515
|
|
},
|
|
"writtenOn": "2021-01-20T07:52:57Z",
|
|
"side": 1,
|
|
"message": "same here, i don\u0027t see what you are trying to test here.\nThe constant is passed to the RunnerConfig, if we want to be sure that there is no override here, we need to check what has been passed to the RunnerConfig",
|
|
"range": {
|
|
"startLine": 509,
|
|
"startChar": 0,
|
|
"endLine": 513,
|
|
"endChar": 88
|
|
},
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": true
|
|
},
|
|
{
|
|
"key": {
|
|
"uuid": "64dc01a9_d23b9132",
|
|
"filename": "validations_libs/tests/test_ansible.py",
|
|
"patchSetId": 8
|
|
},
|
|
"lineNbr": 513,
|
|
"author": {
|
|
"id": 32926
|
|
},
|
|
"writtenOn": "2021-01-20T09:06:26Z",
|
|
"side": 1,
|
|
"message": "as above...",
|
|
"parentUuid": "7aa023d9_2dde2911",
|
|
"range": {
|
|
"startLine": 509,
|
|
"startChar": 0,
|
|
"endLine": 513,
|
|
"endChar": 88
|
|
},
|
|
"revId": "2b5f2028f0c8e0716b0833a70da0ba756bc13d6c",
|
|
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
|
"unresolved": false
|
|
}
|
|
]
|
|
} |