print!

The intent of the side-effect url_for_test function (defined in this test) is not to make the test behaves exactly the same as the production code. This would not make sense since this side-effect function is not the production code.

I just want to assert that the enrich mechanism works for a revision without children or parent entry with a relative coherence to the production code (that's why i wanted some sensible similarly like urls than production code here but again it's not necessary).

Furthermore, i see that you changed the test code to factorize (which is often good) but neither the input nor the output changed. So those modifications do not add anything to the test.

If anything, they add complexity to the maintenance (for the next person).
Can you please remove those modifications on that test only.

This time, in this context, i agree this is necessary ^^

if the else statement is no longer used, please remove it.
Also, if it is removed, the side effect function become simply url_for_context_test.

It'll simplify later maintenance.
^^

Same remark as previous comment.

Note: This was to be symmetric with the generator below ^^

Note: It was a fix because the serialization step (swh.web.ui.backend layer discussion with swh-storage) was not ok with map.

Please, prefix it with _ as you did for the other @nottest util test function.
It is a good idea.

I'm sorry but what does that mean?

imm for immediate, ie the direct child of the revision, which we can obtain from the breadcrumbs. In hindsight there's no reason not to write "immediate" instead.

At first sight, this has become a hairy ball, can't you please try to split it up some more?
(... Even when reading it, i don't quite grasp it.)

You have defined quite a lot of tests to cover it so it should not be a problem to try and split it (even though it's unit tests).

Ok, thanks.
direct is shorter and that's exactly what you used to define it to me ^^
I'd say that's a sign ^^

if context: should be sufficient.

Well, like i said previously. Please try to split this.

I already thought it was big before this (and there were no nested ifs).
So you can imagine what i think now ^^

Not only the use cases are important, we also need to maintain it.
So the:

• code's intent must be clear (i don't find it is right now)
• code's coverage must be fine (it is!)

Hints for you to split up:

• you already have defined tests so don't touch them for now, they are your specs (if all use cases are covered that is).

At the end of it all, it should still work (if you keep the same endpoint as point of origin).

Question hints:

• What are all possible contexts?
• Can't you use one function per context? (the answer can be anything ^^)

ahhhh, we are missing currying here... (as in ocaml or haskell ^^)

You could actually have defined a closure lambda passing the context.
Something like (not tested):

def _enrich_revision(revision, context=context):  # closes over the context 
    return utils.enrich_revision(revision, context)

return _api_lookup(
    sha1_git,
    lookup_fn=service.lookup_revision,
    error_msg_if_not_found='Revision with sha1_git %s not'
                           ' found.' % sha1_git,
    enrich_fn=_enrich_revision)

throught?

his/her.

is it a todo or fixme?

Sorry to not be clearer, just pass self._url_for_context_test to mock_flask.url_for.side_effect instruction below.

That's just me being dense, no worries ^^

I already liked it better ^^, thanks.

You could already return here so that you don't have too many indentation in the second branch.

Please, extract a variable child_id (or something) before the if (it's used in both branched condition).
As we do not have types, this permits to at least explains the intent of the code snippet.
This also permits to avoid mistyping ^^

Why would it be the last sha1 that is the direct children?
Sorry if it's obvious again, i just don't see it.

Please, again, name this context.split thingy.
It makes for a better readability.

Since you use it so much, i think it's a good idea to have an utility function with an explicit name for that.
Its goal would be to split the context (as per input of the api) and return the list of sha1s.
(The order of that list should respect the order of the string context.)
What do you think?

The path that contains the navigation breadcrumbs is constructed from the oldest revision to the most recent. So when you're looking up the last element in path, you get the very next revision in the path you took through the revisions.

duh! ^^
Indeed.