Skip to content

Remake document's display checking test cases#259

Merged
mykhaly merged 16 commits intoopenprocurement:develfrom
mykhaly:devel
Aug 5, 2016
Merged

Remake document's display checking test cases#259
mykhaly merged 16 commits intoopenprocurement:develfrom
mykhaly:devel

Conversation

@mykhaly
Copy link
Contributor

@mykhaly mykhaly commented Jul 19, 2016

Main things are to use ${doc_id} instead of ${url} to find proper document and to remove file after uploading.


This change is Reviewable

@selurvedu
Copy link
Contributor

В загальному твій підхід мені подобається, крім останнього коміту.


Reviewed 2 of 2 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 9 unresolved discussions.


a discussion (no related file):
Ти замінюєш деякі ківорди на інші (новостворені), а що з старими? Чи є серед тих, які ти замінив, такі, які вже не використовуються і їх можна буде видалити? Я знайшов один (написав для нього коментар), але далі і не шукав. Перевір, будь ласка.


a discussion (no related file):
Ти робиш правильно, потрохи зменшуючи кількість спагеті-коду (:+1: :+1: :+1:). Пропоную піти далі і змінити останній коміт так, щоб драйверам брокерів не потрібно було зчитувати вміст файлу, щоб вони натомість при збереженні файлу давали не його вміст, а лише str, який містить шлях, де вони той файл зберегли[0], а вже в robot_tests, а не в драйвері, відбувалось його зчитування і порівняння. Якщо ти це зробиш, то мені не доведеться це робити в #251. :)

Як варіант – драйвери можуть повертати лише filename, а місце для збереження буде обговорено заздалегідь – наприклад, ${OUTPUT_DIR} (або підкаталог в ньому –${OUTPUT_DIR}${/}documents, правда, його треба буде спершу створити). Який варіант кращий – як на мене, другий, але я ще не придумав, які в нього недоліки.

[0]: Або якщо їм так зручніше – file-like object. Поки що наш код це вміє, але якщо продовження підтримки FLO буде ускладнювати код, то можна підримку відпиляти.


op_robot_tests/tests_files/base_keywords.robot, line 241 [r2] (raw file):

  ${number_of_lots}=  Get Length  ${USERS.users['${tender_owner}'].initial_data.data.lots}
  :FOR  ${lot_index}  IN RANGE  ${number_of_lots}
  \  Звірити відображення поля title документа ${USERS.users['${tender_owner}'].lots_documents[${lot_index}].doc_id} із ${USERS.users['${tender_owner}'].lots_documents[${lot_index}].filepath} для користувача ${username}

А от в openProcedure.robot в тебе ['filepath'], а не .filepath.


op_robot_tests/tests_files/openProcedure.robot, line 470 [r3] (raw file):

  ...      ${USERS.users['${viewer}'].broker}
  ...      add_lot_doc  level2
  Отримати посилання на документацію до всіх лотів для користувача ${viewer}

Цей ківорд більше ніде не використовується.


op_robot_tests/tests_files/service_keywords.py, line 417 [r1] (raw file):

def get_id_from_doc_name(name):
    name = name.split('/')

Заміни '/' на os.path.sep.


op_robot_tests/tests_files/service_keywords.py, line 418 [r1] (raw file):

def get_id_from_doc_name(name):
    name = name.split('/')
    name = name[2]

Чому 2? Бо /tmp/file.ext? А якщо mktemp створив файл не в /tmp/? Якщо так, заміни 2 на -1.


op_robot_tests/tests_files/service_keywords.py, line 416 [r3] (raw file):

def get_document_by_id(data, doc_id):

Може це варто перенести в openprocurement_client_helper.py?


op_robot_tests/tests_files/brokers/openprocurement_client.robot, line 46 [r2] (raw file):

Отримати інформацію із документа
  [Arguments]  ${username}  ${tender_uaid}  ${doc_id}  ${field}
  ${tender}=  openprocurement_client.Пошук тендера по ідентифікатору  ${username}  ${tender_uaid}

Якщо ми послідовно отримуємо різну інформацію із документа (тим більше, з кількох), то чому б не закешувати ті дані? В брокерів, які працюють через сайт, є перевага – відкрита сторінка в браузері, з якої можна читати підряд різні дані і рефрешити лише коли потрібно. Чи це ускладнить код?


op_robot_tests/tests_files/brokers/openprocurement_client.robot, line 57 [r4] (raw file):
BTW

Отримувати URL самостійно з тендера знаючи лише його id, який зберігається в тайтлі

👍 👍 👍


Comments from Reviewable

@mykhaly
Copy link
Contributor Author

mykhaly commented Jul 20, 2016

Що не так з останнім комітом?


Review status: 0 of 4 files reviewed at latest revision, 10 unresolved discussions.


a discussion (no related file):

Previously, selurvedu wrote…

Ти замінюєш деякі ківорди на інші (новостворені), а що з старими? Чи є серед тих, які ти замінив, такі, які вже не використовуються і їх можна буде видалити? Я знайшов один (написав для нього коментар), але далі і не шукав. Перевір, будь ласка.

Та, забув їх повидаляти. Видалю.

a discussion (no related file):

Previously, selurvedu wrote…

Ти робиш правильно, потрохи зменшуючи кількість спагеті-коду (:+1: :+1: :+1:). Пропоную піти далі і змінити останній коміт так, щоб драйверам брокерів не потрібно було зчитувати вміст файлу, щоб вони натомість при збереженні файлу давали не його вміст, а лише str, який містить шлях, де вони той файл зберегли[0], а вже в robot_tests, а не в драйвері, відбувалось його зчитування і порівняння. Якщо ти це зробиш, то мені не доведеться це робити в #251. :)

Як варіант – драйвери можуть повертати лише filename, а місце для збереження буде обговорено заздалегідь – наприклад, ${OUTPUT_DIR} (або підкаталог в ньому –${OUTPUT_DIR}${/}documents, правда, його треба буде спершу створити). Який варіант кращий – як на мене, другий, але я ще не придумав, які в нього недоліки.

[0]: Або якщо їм так зручніше – file-like object. Поки що наш код це вміє, але якщо продовження підтримки FLO буде ускладнювати код, то можна підримку відпиляти.

Я думав над цим. Чисто з точки зору логіки краще було би, якби майданчики повертали нам FLO. Але, я з файлами в пітоні майже не працював, тому не знаю, які можуть підводні камені. В такому випадку можна було би купу всього додаткового перевірити - час модифікації, розмір файлу і ше якийсь брєд. `${OUTPUT_DIR}` можна запхати для кожного брокера в `${USERS.users['${username}']}` і задавати в `brokers.yaml`. Або змусити майданчиків качати файли в `${test_output}`, наприклад.

a discussion (no related file):
Прочитав коментарі. Зрозумів, що майже про всі зауваження я думав під час того, як працював над цим. Але в якийсь момент думка перескочила на щось інше і я забув, що планував, або забив на то, бо був змучений :(
Дякую за рев'ю.


op_robot_tests/tests_files/base_keywords.robot, line 241 [r2] (raw file):

Previously, selurvedu wrote…

А от в openProcedure.robot в тебе ['filepath'], а не .filepath.

Тут я вирішив ризикнути і повірити в те, що `${lots_documents[index]}` - munch. А там вирішив працювати "навєрняка" :) Виправив. На майбутнє - всюди, де є dict, зробити munch.

op_robot_tests/tests_files/openProcedure.robot, line 470 [r3] (raw file):

Previously, selurvedu wrote…

Цей ківорд більше ніде не використовується.

Видалив.

op_robot_tests/tests_files/service_keywords.py, line 417 [r1] (raw file):

Previously, selurvedu wrote…

Заміни '/' на os.path.sep.

Done.

op_robot_tests/tests_files/service_keywords.py, line 418 [r1] (raw file):

Previously, selurvedu wrote…

Чому 2? Бо /tmp/file.ext? А якщо mktemp створив файл не в /tmp/? Якщо так, заміни 2 на -1.

Хтів це зробити зразу, забув. Done.

op_robot_tests/tests_files/service_keywords.py, line 416 [r3] (raw file):

Previously, selurvedu wrote…

Може це варто перенести в openprocurement_client_helper.py?

Я не знаю, яке в нас розмежування між `openprocurement_client_helper.py` і `service_keywords.py`. А дещо, здається, є і в `initial_data.py`. Тре придумати, як їх розмежувати.

op_robot_tests/tests_files/brokers/openprocurement_client.robot, line 46 [r2] (raw file):

Previously, selurvedu wrote…

Якщо ми послідовно отримуємо різну інформацію із документа (тим більше, з кількох), то чому б не закешувати ті дані? В брокерів, які працюють через сайт, є перевага – відкрита сторінка в браузері, з якої можна читати підряд різні дані і рефрешити лише коли потрібно. Чи це ускладнить код?

Хотів так зробити, за аналогом до `Отримати дані з тендера`. Але зрозумів, що не вийде. Бо в `Отримати дані з тендера` ми маємо повний шлях до поля і можемо перевірити, чи такі дані вже є в кеші, та й знаємо куди писати. А тут в нас є тільки якась частина заголовку документа, немає його індексу, і якщо прикрутити ще отримання індексу, то код виглядатиме в рази складніше.

Comments from Reviewable

@selurvedu
Copy link
Contributor

Що не так з останнім комітом?

Оце:

Пропоную піти далі і змінити останній коміт


Reviewed 4 of 4 files at r5, 3 of 3 files at r6, 1 of 1 files at r7, 3 of 3 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, 2 of 2 files at r11, 1 of 1 files at r12.
Review status: all files reviewed at latest revision, 7 unresolved discussions.


a discussion (no related file):

Previously, mykhaly (Yurii Mykhalchuk) wrote…

Я думав над цим. Чисто з точки зору логіки краще було би, якби майданчики повертали нам FLO. Але, я з файлами в пітоні майже не працював, тому не знаю, які можуть підводні камені. В такому випадку можна було би купу всього додаткового перевірити - час модифікації, розмір файлу і ше якийсь брєд.
${OUTPUT_DIR} можна запхати для кожного брокера в ${USERS.users['${username}']} і задавати в brokers.yaml. Або змусити майданчиків качати файли в ${test_output}, наприклад.

Так про це й мова. `${OUTPUT_DIR}` – це [automatic variable](http://robotframework.org/robotframework/3.0/RobotFrameworkUserGuide.html#automatic-variables) в Robot Framework. Каталог, який задається опцією `-d`/`--outputdir` (by default це в нас `test_output`).

a discussion (no related file):

Previously, mykhaly (Yurii Mykhalchuk) wrote…

Прочитав коментарі. Зрозумів, що майже про всі зауваження я думав під час того, як працював над цим. Але в якийсь момент думка перескочила на щось інше і я забув, що планував, або забив на то, бо був змучений :(
Дякую за рев'ю.

:)

op_robot_tests/tests_files/keywords.robot, line 545 [r12] (raw file):

Звірити поле скарги із значенням
  [Arguments]  ${username}  ${tender_uaid}  ${given_value}  ${field_name}  ${complaintID}  ${award_index}=${None}
  ${received_value}=  Run as  ${username}  Отримати інформацію із скарги  ${tender_uaid}  ${complaintID}  ${field_name}  ${award_index}

Що тут відбувається?


op_robot_tests/tests_files/openProcedure.robot, line 896 [r11] (raw file):

  ${doc_id}=  get_id_from_doc_name  ${USERS.users['${provider}'].claim_data.document}
  ${right}=  Run As  ${viewer}  Отримати інформацію із документа до скарги  ${TENDER['TENDER_UAID']}  ${USERS.users['${provider}'].claim_data.complaintID}  ${doc_id}  title
  Порівняти об'єкти  ${USERS.users['${provider}'].claim_data.document}  ${right}

Мені здається, чи ти порівнюєш document (а не його title) і title?


op_robot_tests/tests_files/service_keywords.py, line 416 [r3] (raw file):

Previously, mykhaly (Yurii Mykhalchuk) wrote…

Я не знаю, яке в нас розмежування між openprocurement_client_helper.py і service_keywords.py. А дещо, здається, є і в initial_data.py. Тре придумати, як їх розмежувати.

Я вважаю, що варто кидати в `helper.py` те, чим точно не буде користуватись ніхто крім `openprocurement_client.robot`. Я не впевнений щодо цієї функції, як думаєш?

op_robot_tests/tests_files/service_keywords.py, line 417 [r7] (raw file):

def get_document_by_id(data, doc_id):
    for document in data:

BTW
👍, не треба створювати зайву структуру даних.


op_robot_tests/tests_files/brokers/openprocurement_client.robot, line 666 [r11] (raw file):

  ${document}=  get_document_by_id  ${complaints[${complaint_index}].documents}  ${doc_id}
  Log  ${document}
  [Return]  ${document['${field_name}']}

BTW
👍, стало простіше.


Comments from Reviewable

@mykhaly
Copy link
Contributor Author

mykhaly commented Jul 21, 2016

Review status: all files reviewed at latest revision, 4 unresolved discussions.


a discussion (no related file):

Previously, selurvedu wrote…

Так про це й мова. ${OUTPUT_DIR} – це automatic variable в Robot Framework. Каталог, який задається опцією -d/--outputdir (by default це в нас test_output).

Тоді варто робити отак: Всім майданчикам в ключовому слові `Отримати документ` треба скачати файл в `${OUTPUT_DIR}` і повернути з ключового слова тільки назву файлу (на випадок, якщо майданчик вирішить файли переіменовувати і перед тим відображення заголовку не пройде). Потім ми вже цей файл знайдемо своїм кодом, відкриємо і перевіримо вміст. Проблема: Якщо колись вирішимо перевіряти відображення вмісту документа для всіх користувачів, то всі будуть качати файл з однаковим іменем, і прийдеться якось вирішувати питання з переіменуванням чи перебиванням файлу.

op_robot_tests/tests_files/keywords.robot, line 545 [r12] (raw file):

Previously, selurvedu wrote…

Що тут відбувається?

Отримується значення поля `${field_name}` скарги `${complaintID}` і порівнюється з `${given_value}`. Все те, що видалено, існувало для того, щоб в цьому ключовому слові можна було отримати поле документа до скарги, шляхом передачі `document.title` (чи якогось іншого поля, наприклад `document.id`) в якості `${field_name}`.

op_robot_tests/tests_files/openProcedure.robot, line 896 [r11] (raw file):

Previously, selurvedu wrote…

Мені здається, чи ти порівнюєш document (а не його title) і title?

Нє, тут все ок. В `document` лежить назва створеного файлу. (Впринаймі тести пройшли, я перевіряв).

op_robot_tests/tests_files/service_keywords.py, line 416 [r3] (raw file):

Previously, selurvedu wrote…

Я вважаю, що варто кидати в helper.py те, чим точно не буде користуватись ніхто крім openprocurement_client.robot. Я не впевнений щодо цієї функції, як думаєш?

Якщо таким чином розділяти, то я за. Навряд чи майданчикам потрібен доступ до цієї функції.

Comments from Reviewable

@mykhaly
Copy link
Contributor Author

mykhaly commented Jul 22, 2016

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions.


op_robot_tests/tests_files/service_keywords.py, line 417 [r7] (raw file):

Previously, selurvedu wrote…

BTW
👍, не треба створювати зайву структуру даних.

Фігню я зробив, треба переглядати всі документи в тендері, а не тільки `tender_data.data.documents`. Тоді `get_document_by_id` можна буде використовувати для отримання документу як з `tender_data.data.documents`, так і `tender_data.data.complaints[index].documents` і `tender_data.data.awards[index].documents`.

Comments from Reviewable

@selurvedu
Copy link
Contributor

selurvedu commented Jul 22, 2016

Reviewed 5 of 6 files at r13, 1 of 1 files at r14, 1 of 1 files at r15, 1 of 5 files at r16, 3 of 4 files at r17, 1 of 1 files at r18, 1 of 1 files at r19, 1 of 1 files at r20, 1 of 1 files at r21, 2 of 2 files at r22, 1 of 1 files at r23, 1 of 1 files at r24.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


a discussion (no related file):

Previously, mykhaly (Yurii Mykhalchuk) wrote…

Тоді варто робити отак:
Всім майданчикам в ключовому слові Отримати документ треба скачати файл в ${OUTPUT_DIR} і повернути з ключового слова тільки назву файлу (на випадок, якщо майданчик вирішить файли переіменовувати і перед тим відображення заголовку не пройде). Потім ми вже цей файл знайдемо своїм кодом, відкриємо і перевіримо вміст.
Проблема:
Якщо колись вирішимо перевіряти відображення вмісту документа для всіх користувачів, то всі будуть качати файл з однаковим іменем, і прийдеться якось вирішувати питання з переіменуванням чи перебиванням файлу.

OK, було вирішено наступне: Оскільки ми проганяємо тести лише для одного брокера за раз (не рахуючи openprocurement_client.robot), ця проблема нас поки що не зачіпає.

op_robot_tests/tests_files/openProcedure.robot, line 889 [r24] (raw file):

документації

документа до


op_robot_tests/tests_files/openProcedure.robot, line 906 [r24] (raw file):

create_lot_claim

create_tender_claim


op_robot_tests/tests_files/service_keywords.py, line 417 [r7] (raw file):

Previously, mykhaly (Yurii Mykhalchuk) wrote…

Фігню я зробив, треба переглядати всі документи в тендері, а не тільки tender_data.data.documents. Тоді get_document_by_id можна буде використовувати для отримання документу як з tender_data.data.documents, так і tender_data.data.complaints[index].documents і tender_data.data.awards[index].documents.

LGTM

op_robot_tests/tests_files/brokers/openprocurement_client.robot, line 59 [r22] (raw file):

  # ${contents} is never used and exists only because get_file returns tuple
  ${contents}  ${filename}=  Call Method  ${USERS.users['${username}'].client}  get_file   ${tender}   ${document.url}   ${token}
  ${new_path_to_file}=  Move File  ${filename}  ${OUTPUT_DIR}

get_file не пише на диск. Треба додати зберігання файлу на диск замість get_file і забрати Move File.


Comments from Reviewable

@mykhaly
Copy link
Contributor Author

mykhaly commented Jul 22, 2016

Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions.


a discussion (no related file):

Previously, selurvedu wrote…

OK, було вирішено наступне:
Оскільки ми проганяємо тести лише для одного брокера за раз (не рахуючи openprocurement_client.robot), ця проблема нас поки що не зачіпає.

Точніше тому, що завантаження кожного окремого документа відбувається тільки одним брокером (рахуючи openprocurement_client.robot).

op_robot_tests/tests_files/openProcedure.robot, line 889 [r24] (raw file):

Previously, selurvedu wrote…

документації

документа до

Done.

op_robot_tests/tests_files/openProcedure.robot, line 906 [r24] (raw file):

Previously, selurvedu wrote…

create_lot_claim

create_tender_claim

Done.

op_robot_tests/tests_files/brokers/openprocurement_client.robot, line 59 [r22] (raw file):

Previously, selurvedu wrote…

get_file не пише на диск. Треба додати зберігання файлу на диск замість get_file і забрати Move File.

Done.

Comments from Reviewable

mykhaly added 15 commits July 27, 2016 11:10
Return path to created file, basename of created file and content of
created file instead of name of created file (and path to it at the same
time). It is made in order to delete file after uploading and save all
useful data in Robot Framework variables.
Add ${doc_id} variable and get_id_from_doc_name function.
Use updated `create_fake_doc` function.
Use better names for variables.
Use id from title instead of url as data given to broker to find the document.
Add new keyword `Отримати інформацію із документа` and use it instead of
`Отримати інформацію із тендера`.
Add `get_document_by_id` and `get_file_basename` functions, because they
are needed in `Отримати документ` keyword.
Remove `get_file_contents` function call.
Remake `Отримати документ` keyword: now it returns only name of
downloaded by broker file in lieu of file content and path. Moreover,
`${url}` arguments is replaced with `${doc_id}` so as to give to broker
better data about document to download.
Now is used Robot Framework keyword `Get File`, so it is redundant
In order to work with new arguments returned from `create_fake_doc()`.
Also add `${doc_id}`, `${doc_name}` and `${doc_content}` to `${claim_data}`
and `Remove File` it the end of keywords body.
Call `Отримати інформацію із документа до скарги` directly from test case, without any shell.
Rename `Отримати поле документації до скарги` -> `Отримати інформацію із документа до скарги`
and remake algorithm inside it.
Also update saving those data returned from `create_fake_doc` into
variable and add removing of file after uploading.
Remove useles row `${number_of_lots}=  Get Length
${USERS.users[].initial_data.data.lots}` and use previously saved data
for checking document contents instead of reading that from file.
To work with changed `create_fake_doc` function. Also remove file after uploading,
like in others keywords from base_keywords.robot.
@mykhaly
Copy link
Contributor Author

mykhaly commented Aug 4, 2016

Merge only after #33, #264 and #265.

To work with changed `create_fake_doc` function. Also remove file after uploading,
like in others keywords from base_keywords.robot.
@mykhaly mykhaly merged commit 75a5f65 into openprocurement:devel Aug 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants