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

Add support for external test harness #343

Merged
merged 1 commit into from
Jun 9, 2020
Merged

Conversation

thesamet
Copy link
Contributor

@thesamet thesamet commented Jun 8, 2020

I am building PGV support for ScalaPB. As ScalaPB offers many ways for users to customize the generated code and the generator is written in Scala, it would be impractical to add ScalaPB support directly in here.

We have started to put some effort in https://github.com/scalapb/scalapb-validate and it would be nice to be able to run PGV's test harness against scalapb-validate. To accomplish that, I would like to be able to specify an external program to the harness.

See #322

Copy link
Contributor

@akonradi akonradi left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good. I'm hoping we can generalize some of the function calls so we don't need to pass so many arguments around.

tests/harness/executor/executor.go Outdated Show resolved Hide resolved
tests/harness/executor/executor.go Outdated Show resolved Hide resolved
@thesamet thesamet force-pushed the master branch 3 times, most recently from b26d7c9 to 5972dad Compare June 8, 2020 15:58
Copy link
Contributor

@akonradi akonradi left a comment

Choose a reason for hiding this comment

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

One minor nit and then this should be good to go

tests/harness/executor/executor.go Outdated Show resolved Hide resolved
akonradi
akonradi previously approved these changes Jun 8, 2020
@thesamet
Copy link
Contributor Author

thesamet commented Jun 9, 2020

For reference, this is how this is used right now in scalapb-validate's ScalaHarness.

We inverted the control, so the Scala code calls the harness and keeps a single JVM for the entire test, which make it significantly faster (compared to the approach of launching a new JVM for each case). Though we still have over 100 test errors to fix :)

@akonradi
Copy link
Contributor

akonradi commented Jun 9, 2020

That makes sense. One of the reasons the test is so slow is that the Java harness takes forever to run since it also starts a new JVM every time.

@thesamet can you merge master?

@thesamet
Copy link
Contributor Author

thesamet commented Jun 9, 2020

Sure, I rebased and squashed this into a single commit.

@akonradi akonradi merged commit 1457b8e into bufbuild:master Jun 9, 2020
@akonradi akonradi mentioned this pull request Jun 9, 2020
@akonradi
Copy link
Contributor

akonradi commented Jun 9, 2020

Thanks!

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.

2 participants