tests: unit tests for workflows/tasks/submission

[jacquerie]

Addresses #1773

• add_note_entry
• close_ticket
• create_ticket
• filter_keywords
• prepare_files
• prepare_keywords
• remove_references
• reply_ticket
• submit_rt_ticket
• send_robotupload
• wait_webcoll

[jacquerie]

Bugs uncovered while writing these tests:

• Most log calls use format instead of what is recommended in global: correct logging #1371.
• Most log calls are not unicode-safe. Some of these bugs were already solved in global: ensure unicode handling in all user-facing code #2199.
• It's unsafe to call strip on something that might be None:

  inspire-next/inspirehep/modules/workflows/tasks/submission.py

  Line 158 in 5fdbd8b

  body = obj.extra_data.get("reason").strip()

• It's highly probable that the exception in

  inspire-next/inspirehep/modules/workflows/tasks/submission.py

  Lines 222 to 223 in 5fdbd8b

  	if ticket["Status"] != "resolved":
  	raise

  is impossible to trigger.

[jacquerie]

Design problems encountered while writing these tests:

• This whole thing reeks of spaghetti code: relying on some task to write a certain thing in extra_data so that some task at some later point will pick it up is error-prone and is guaranteed to break in the future when tasks are added/deleted/reordered.

• These function have too many responsibilities, which results in too many mocks that have to be applied to them while testing. For example reply_ticket contains:

  • some logic* to get the ticket_id on which it should operate;
  • some logic to figure out if the user actually exists;
  • some logic (with fallbacks) to build the ticket body;
  • some logic to fetch an RT instance;
  • finally, some logic to reply to a ticket.

  Note that all these items also have fallback logic to log and bail out if any of these steps fail, effectively duplicating the number of responsibilities of this function.

* Note also that this logic is subtly different from the one in close_ticket.

[kaplun]

relying on some task writing a certain thing in extra_data so that some task at some later point will pick it up is error-prone and is guaranteed to break in the future when tasks are added/deleted/reordered.

Given the workflow engine we are employing, what better pattern would you propose?

[jacquerie]

Given the workflow engine we are employing, what better pattern would you propose?

Enforce dependencies in code. If task A logically requires task B to be run before it (for example: B is a machine learning algorithm, A persists its results in the record) this must be enforced in code. How? Strong conventions on naming things, ban the use of patterns like extra_data.get(...).

[kaplun]

Strong conventions on naming things, ban the use of patterns like extra_data.get(...).

Actually that makes me think that the object passed around doesn't need to be really a dictionary, so we could pass an actual object with dedicated properties/methods that can then enforce the conventions.

Only issue would be serialization in the DB. But shouldn't be impossible, to just serialize as dictionary and reinstantiate back as full-fledged object.

[kaplun]

Or more simply, indeed we can have setters and getters that receive the obj as first arguments, allowing for still keeping the obj as a simple dict, but hiding the complexity of writing/accessing special extra keys...

[david-caro]

ban the use of patterns like extra_data.get(...)

I'm curious, why do you think that doing that (banning the usage of .get) is enforcing in code dependencies between tasks?

[jacquerie]

I'm curious, why do you think that doing that (banning the usage of .get) is enforcing in code dependencies between tasks?

To avoid failing silently. The problem is in patterns like

foo = extra_data.get('foo')
if foo:
    ...

because this is not enforcing a dependency on foo, so if the task that populates it starts failing tomorrow we are never going to see it. Things should fail loudly and clearly.

[david-caro]

That can be easily avoided with extra_data['foo'] :)

[jacquerie]

That was my point!

[david-caro]

Sorry, I missed that point XS

[jacquerie]

• It's unsafe to call strip on something that might be None:

  inspire-next/inspirehep/modules/workflows/tasks/submission.py

  Line 158 in 5fdbd8b

  body = obj.extra_data.get("reason").strip()

...and in fact: https://sentry.cern.ch/inspire-sentry/inspire-labs/group/822209/

[jacquerie]

Victim of #2385.