Import wellknown types (fixes #9)

[boukeversteegh]

This PR supports a few google well known types (struct, empty, value and listvalue).

This PR's Approach:

• make sure google protobuf's messages are imported correctly

Alternative approach:

• compile from google protobuf .proto source and generate them also in the output directory, and include those

The alternative approach may be a better one in the long term, because it will automatically support all google types, and the objects will be betterproto.Message instead of googles internal type

[nat-n]

is this sufficient for it to get included in the test run?

[boukeversteegh]

yup~ :-D

[boukeversteegh]

It will get included in the test run, meaning that the service will be instantiated.
actually, whether that's a sufficient test is another thing..

i have tested in a separate unit test that the import statements are generated correctly, but i'm having problems running it in the same go with the other tests.

the datadescriptor pool from google protobuf is filled twice with the same protobufs, for some reason. if i run the test separately, it works, but not together with the other tests.

i know from fixing #40 that returning the google-compiled wellknown value works, as long as its imported, so i think this will be good, but i haven't done a manual end-to-end-test

[boukeversteegh]

the test i have problems with, and haven't committed, is this one:

betterproto/tests/test_get_ref_type.py

import pytest

from ..plugin import get_ref_type


@pytest.mark.parametrize(
    ['google_type', 'expected_name', 'expected_import'], [
        ('.google.protobuf.Empty', 'google_protobuf_empty_pb2.Empty',
         'import google.protobuf.empty_pb2 as google_protobuf_empty_pb2'),
        ('.google.protobuf.Struct', 'google_protobuf_struct_pb2.Struct',
         'import google.protobuf.struct_pb2 as google_protobuf_struct_pb2'),
        ('.google.protobuf.ListValue', 'google_protobuf_struct_pb2.ListValue',
         'import google.protobuf.struct_pb2 as google_protobuf_struct_pb2'),
        ('.google.protobuf.Value', 'google_protobuf_struct_pb2.Value',
         'import google.protobuf.struct_pb2 as google_protobuf_struct_pb2'),
    ])
def test_import_google_wellknown_types(google_type: str, expected_name: str, expected_import: str):
    imports = set()
    name = get_ref_type(package='', imports=imports, type_name=google_type)

    assert name == expected_name
    assert imports.__contains__(expected_import)

When running all tests, this error is raised by google protobuf lib

>       return _message.default_pool.AddSerializedFile(serialized_pb)
E       TypeError: Couldn't build proto file into descriptor pool!
E       Invalid proto descriptor for file "repeatedpacked.proto":
E         Test: "Test" is already defined in file "double.proto".

[nat-n]

Hmm, weird. It would be preferable to have full functional coverage, even if as a stopgap we have to mark it as a test that has to be run in isolation, so isn't included in the main test run. What do you think?

[boukeversteegh]

yeah i would also prefer full coverage!

we could also concider to isolate the test_binary_compatibility instead, because that's the one causing problems (probably because of libmodule.import), as i'm expecting we'll need more unit tests that import plugin.py, and each of them will conflict with test_binary_compatibility

we can update the CI to run both test sets separately

[boukeversteegh]

any idea how to mark the tests?
we could probably add groups/tags or something, but it does mean that contributors will need to know to run these tests separately.

would be great if we could let the standard pytest command run without problems 🤔

[nat-n]

Maybe worthwhile to make the input test loading a bit more sophisticated to handle this stuff more smoothly.

I'm thinking instead of expecting there to be a module member called Test, make it scan the members and import anything that starts with Test. Should be easy?

[nat-n]

As for marking the tests as separate test runs (requiring separate python runtimes), there might be a pytest feature or plugin to handle this otherwise having a test task (pipenv script, make target or wtv) that does the right tging should cover it. Though maybe solving the naming problem with smarter import logic makes the problem go away?

[boukeversteegh]

That could work, and i would like it to support Test* messages instead of mandatory names..

although using it as the only fix for this conflict, would make it mandatory that all proto messages have unique names... would be nice if we can avoid that

[Evgenus]

Thanks for this PR. I trapped into that issue a hour ago. Please add also import "google/protobuf/any.proto";

[Evgenus]

There is a bug

syntax = "proto3";

package test;

import "google/protobuf/struct.proto";

message TestMessage {
    google.protobuf.Value value = 1;
}

# Generated by the protocol buffer compiler.  DO NOT EDIT!
# sources: test.proto
# plugin: python-betterproto
from dataclasses import dataclass

import betterproto

import google.protobuf.struct_pb2 as google_protobuf_struct_pb2


@dataclass
class TestMessage(betterproto.Message):
    value: google_protobuf_struct_pb2.Value = betterproto.message_field(
        1, wraps=betterproto.TYPE_
    )

TYPE_ is not defined

[boukeversteegh]

Thank you for testing this PR @Evgenus . I will include a fix for that bug in this pr

[boukeversteegh]

Closed in favor of #78