Build #705 failed (commit: c10b5be056a2f245f1d4ec67ef7082ebed745020).
Rebase or make new commits to rebuild.

Build #706 failed (commit: c10b5be056a2f245f1d4ec67ef7082ebed745020).
Rebase or make new commits to rebuild.

Rather than repeat this logic, how about calling self._add_base_module_provides(solvable, mmd) before self.solvable_provides(solvable, n, s, str(v)) and have self._add_base_module_provides(solvable, mmd) return a boolean denoting if the stream version was added as a provide?

Thanks @mprahl, updated.

Build #710 failed (commit: ca22632f1f192c62f9aa465a29e1680ccf9aee76).
Rebase or make new commits to rebuild.

@breilly this looks great. Could explain the return value in the docstring for _add_base_module_provides?

Afterwards, this is good to merge.

Build #711 failed (commit: eb077643ccfe2367c00e495100f936e101cb0b47).
Rebase or make new commits to rebuild.

I know this is nitpicking, but the purpose of the returned boolean is not dictated by this method, that is determined whatever code uses that boolean. I would suggest just stating what the boolean is for.

For instance, you could say: A boolean that is True if a provides for the stream version was added to the input solvable

Seems existing tests does not catch the issue reported in #1334. @breilly could you please add a test for this fix?

Build #721 failed (commit: 8b854a9c12861bacd79676d615c5ec3e8a11c90a).
Rebase or make new commits to rebuild.

@breilly how about another test like this to verify that the correct provides were added to the solvable?

@pytest.mark.parametrize(
    "nsvc, expected",
    (
        ("platform:f28:3:c0", {"module(platform)", "module(platform:f28) = 28.0"}),
        ("platform:latest:5:c8", {"module(platform)", "module(platform:latest) = 5"}),
    ),
)
def test_base_module_provides(self, nsvc, expected):
    self.mmd_resolver.add_modules(self._make_mmd(nsvc, {}))
    ns = nsvc.rsplit(":", 2)[0]
    provides = self.mmd_resolver.solvables[ns][0].lookup_deparray(solv.SOLVABLE_PROVIDES)
    assert {str(provide) for provide in provides} == expected

Thanks @mprahl, was having difficulty checking the provides directly but that works. Updated.

Seems get_nsvc_as_string[1] works for this purpose.

[1] https://sgallagh.fedorapeople.org/docs/libmodulemd/latest/modulemd-2.0-ModulemdModuleStream.html#modulemd-module-stream-get-nsvc-as-string

Suggest to rename n -> nsvc and req -> requires.

In MBS code, name requires represents the runtime requires of a modulemd generally, and name buildrequires is for the buildtime requires specifically.

The test name could be more specific here. Maybe replaced with def test_provide_base_module_stream_version. Feel free to reword and choose another one that can express the test purpose directly.

Similar with above comment. What is the purpose of this test? It calls add_modules rather than _add_base_module_provides. It'd be better express the test purpose clearly, for example by the name, as much as possible.

If you want to test _add_base_module_provides only, the test should just call it and verify the result and relative behaviors.

This specific test is to verify that add_modules adds the correct provides to the solvable.

This specific test is to verify that add_modules adds the correct provides to the solvable.

Should it be possible to merge test_add_base_module_provides and test_base_module_provides into one, which calls self.mmd_resolver.add_modules, with fixture data specific to different cases?

BTW, I feel _add_base_module_provides should be split into two functions for the purpose of getting result of whether a base module is added as a provides when it has stream version.

Currently, _add_base_module_provides adds a base module as provides in two conditions, 1) if it has stream version and 2) if it has virtual streams in xmd["mbs"]["virtual_streams"]. However, in this PR, the function returns a value to indicate True or False of the part of the operations. It looks a little strange. So, perhaps we can split the function into two, for example,

• provide_base_module_if_has_stream_version
• provide_base_module_with_virtual_streams

then, we can determine whether to add base module as a provides with its n, s, str(v) only with the return value from the first one.

What do you think?

Based on the feedback from @cqi above, seems like we should at least make the following easy changes to the new unit tests:

• rename those parametrized variables as suggested
• make the test name more specific
• use get_nsvc_as_string instead of calculating ourselves

I don't think we need to refactor _add_base_module_provides any further at this time (i.e. the split suggested), but it might make sense to look into this separately.

As for merging the two new unit tests, I'm mostly ambivalent. I'd tend toward leaving them as-is unless there is a real problem with the way they are set up.

@breilly could we get those three easy bits applied at least?

@cqi if I'm off the mark about skipping/deferring the rest, please correct me

Thanks @cqi @mikem, updated.

Could you just use the nsvc directly?

You can use mmd.get_version() here.

Can you use is instead of ==?

@breilly a few minor things to change in test. Once addressed, this looks good to merge.

Thanks @mprahl, updated.

:thumbsup:

Commit 66a2362 fixes this pull-request

Build #732 failed (commit: c2561da).
Rebase or make new commits to rebuild.