Skip to content

fix: move proto test files under scripted#475

Merged
octonato merged 1 commit intomainfrom
rgc/feed-proto-test-files-to-scripted
Sep 23, 2021
Merged

fix: move proto test files under scripted#475
octonato merged 1 commit intomainfrom
rgc/feed-proto-test-files-to-scripted

Conversation

@octonato
Copy link
Copy Markdown
Member

On top of #470.

This brings the proto test file under the scripted tests. I needed to move the files there and then let the java compilation test point to the scripted proto dir.

The reason for that is that the whole project needs to be copied to /tmp when running and we need the files to be copied as well. That said, the scripted tests is now the host for those files.

Comment thread sbt-plugin/src/sbt-test/sbt-akkaserverless/basic/test
Comment thread sbt-plugin/src/sbt-test/sbt-akkaserverless/basic/test
Comment thread sbt-plugin/src/sbt-test/sbt-akkaserverless/basic/build.sbt Outdated
Base automatically changed from rgc/fix-action-provider-compilation to main September 23, 2021 10:48
@octonato octonato force-pushed the rgc/feed-proto-test-files-to-scripted branch from ae5b48c to 111237b Compare September 23, 2021 11:37
@octonato octonato changed the title fix: action provider compilation fix: move proto test files under scripted Sep 23, 2021
@octonato octonato force-pushed the rgc/feed-proto-test-files-to-scripted branch from 111237b to 1eae839 Compare September 23, 2021 12:06
|
| override final def additionalDescriptors: immutable.Seq[Descriptors.FileDescriptor] =
| ${service.fqn.parent.name}Proto.javaDescriptor ::
| ${typeName(service.descriptorObject)}.javaDescriptor ::
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change looks good, but the test trips over it. Likely because the test still uses a 'Java-style' model, I'll PR making that easier.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(PR'ed #481, though not sure anymore it's the same problem)

Copy link
Copy Markdown
Contributor

@raboof raboof left a comment

Choose a reason for hiding this comment

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

I liked having a 'realistic' example in the scripted tests, but I guess we can re-introduce that when we want ;)

Copy link
Copy Markdown
Contributor

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

LGTM, an additional scripted setup we could use is to setup something only for CI and have a job copying proto files from the samples and verify that they compile. Compared to the samples themselves it would validate that the generated unmanaged code compiles.

@octonato octonato force-pushed the rgc/feed-proto-test-files-to-scripted branch from 1eae839 to 165d7c0 Compare September 23, 2021 13:56
@octonato
Copy link
Copy Markdown
Member Author

I liked having a 'realistic' example in the scripted tests, but I guess we can re-introduce that when we want ;)

Are you referring to the shopping-cart that I removed? We can add it back. My thought was that we have enough realistic examples being tested in the samples and that the scripted/compilation tests should be about generating and compiling exhaustively.

LGTM, an additional scripted setup we could use is to setup something only for CI and have a job copying proto files from the samples and verify that they compile.

We will need to make sure that the samples protos don't conflict with each other. That they don't define the same types in the same packages. This is at least true for the counter sample. Same model for value entity and event sourced entity. The customer registry is probably the same. I don't think it's worth the effort to make then not conflict.

Compared to the samples themselves it would validate that the generated unmanaged code compiles.

But this is already happening, no? We do run the tests for the samples. They compile and run.

@patriknw
Copy link
Copy Markdown
Contributor

We will need to make sure that the samples protos don't conflict with each other.

We could setup CI job that run copy and run them one by one.

But this is already happening, no? We do run the tests for the samples. They compile and run.

Not fully testing with an empty project so that we verify that the generated unmanaged sources are correct. The unmanaged sources are in git and implemented by us.

An easier approach to my idea would be to not use scripted, but just have a CI job that removes src/main/ and runs sbt compile. That approach also works for mvn. I'll create a ticket.

@patriknw
Copy link
Copy Markdown
Contributor

Created issue #486 for that additional CI idea.

@octonato octonato merged commit 8abf03b into main Sep 23, 2021
@octonato octonato deleted the rgc/feed-proto-test-files-to-scripted branch September 23, 2021 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants