Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move generated test code for Google.Protobuf.Test to a separate lib #6832

Merged

Conversation

ObsidianMinor
Copy link
Contributor

@ObsidianMinor ObsidianMinor commented Oct 31, 2019

Many of the recent issues with proto2 extensions were accidentally caused by tests passing when code-gen didn't actually compile outside of internal visibility which the Google.Protobuf.Test project has. To prevent mistakes like this in the future, this separates the code-gen from the test project to:

  • Keep the InternalAccessVisibleTo attribute out of the test code-gen
  • Allow us to use a separate minimum framework and standard version when compiling the code-gen

/cc: @jtattermusch

@jtattermusch jtattermusch changed the title Move generated test code for Google.Protobuf.Test to a seperate lib Move generated test code for Google.Protobuf.Test to a separate lib Nov 4, 2019
@jtattermusch
Copy link
Contributor

Test failure:
https://source.cloud.google.com/results/invocations/95078627-231e-41e9-bc4b-665e6e48a041/log
Unittest.cs(891,162): error CS7036: There is no argument given that corresponds to the required formal parameter 'index' of 'Encoding.GetString(byte[], int, int)' [T:\src\github\protobuf\csharp\src\Google.Protobuf.Test.CodeGen\Google.Protobuf.Test.CodeGen.csproj]

@ObsidianMinor
Copy link
Contributor Author

That's fixed with #6828

@ObsidianMinor
Copy link
Contributor Author

If you want I can move it to this PR

and without the internal visibility from the test project (all of which have caused issues in the past).
-->
<PropertyGroup>
<TargetFrameworks>net451;netstandard1.0;netstandard2.0</TargetFrameworks>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: just net45 instead of net451 should be enough?

@jtattermusch
Copy link
Contributor

Looks like C#3 is nearly not enough. I think we can set the LangVersion to 6 in this PR, leave a TODO and take a followup action to make it compatible with C#3 again in a future PR?

Unittest.cs(5142,61): error CS8024: Feature 'expression-bodied property' is not available in C# 3. Please use language version 6 or greater. [/tmp/protobuf/protobuf/csharp/src/Google.Protobuf.Test.CodeGen/Google.Protobuf.Test.CodeGen.csproj]

@ObsidianMinor
Copy link
Contributor Author

ObsidianMinor commented Nov 5, 2019

I could also change it in this PR (or another PR and rebase), since it's a pretty simple change.

private ExtensionSet<T> _Extensions => _extensions;

to

private ExtensionSet<T> _Extensions { get { return _extensions; } }

<Project Sdk="Microsoft.NET.Sdk">

<!--
This CodeGen project is kept seperate from the original test project for many reasons.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: still says "CodeGen" after renaming to TestProtos

typo: s/seperate/separate/

@jtattermusch
Copy link
Contributor

I could also change it in this PR (or another PR and rebase), since it's a pretty simple change.

private ExtensionSet<T> _Extensions => _extensions;

to

private ExtensionSet<T> _Extensions { get { return _extensions; } }

Can we make that change in a separate PR and only use this PR to verify the end result? - It makes things easier to review.
(let's investigate that this is the only problem with C# 3 incompatibility)

@jtattermusch
Copy link
Contributor

I merged #6856, can you update this PR?

Bring .NET Framework target down to 4.5
Rename project in comment
@ObsidianMinor
Copy link
Contributor Author

Rebased 👍 @jtattermusch

@ObsidianMinor
Copy link
Contributor Author

Missed a few things when renaming project. Should've gotten everything now.

@jtattermusch
Copy link
Contributor

Distcheck is failing, but otherwise looking good.

++ '[' '!' -z 'csharp/src/Google.Protobuf.Test.TestProtos/Google.Protobuf.Test.TestProtos.csproj ' ']'
++ echo 'Missing files in EXTRA_DIST: csharp/src/Google.Protobuf.Test.TestProtos/Google.Protobuf.Test.TestProtos.csproj '
Missing files in EXTRA_DIST: csharp/src/Google.Protobuf.Test.TestProtos/Google.Protobuf.Test.TestProtos.csproj
++ exit 1

@ObsidianMinor
Copy link
Contributor Author

Done @jtattermusch

Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@jtattermusch jtattermusch merged commit 6fc04c3 into protocolbuffers:master Nov 11, 2019
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.

None yet

4 participants