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

test: SystemStubs instead of SystemLambda #1092

Merged
merged 6 commits into from
May 11, 2022

Conversation

ed-g
Copy link
Contributor

@ed-g ed-g commented Jan 31, 2022

Summary:

system stubs for #1086 (comment)

Copy link
Contributor

@maximearmstrong maximearmstrong left a comment

Choose a reason for hiding this comment

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

LGTM! @ed-g Could you rerun the checks using the acceptance tests fixed by #1102 ? If everything passes, we should be ready to merge.

@barbeau Do you have any thoughts on the SystemStubs library?

@CLAassistant
Copy link

CLAassistant commented May 10, 2022

CLA assistant check
All committers have signed the CLA.

@ed-g
Copy link
Contributor Author

ed-g commented May 10, 2022

Tests run fine for me now.

Aside: CLA bot seems like it got confused. I haven't un-signed the CLA or anything.

@maximearmstrong
Copy link
Contributor

Tests run fine for me now.

Aside: CLA bot seems like it got confused. I haven't un-signed the CLA or anything.

We had to reconfigure the CLA assistant, this is normal. Could you sign the CLA again?

@ed-g
Copy link
Contributor Author

ed-g commented May 11, 2022

OK

@maximearmstrong
Copy link
Contributor

@ed-g We have configured the repository so that a branch is up to date with master before it can be merged. It seems that your branch is not up to date with master. Could you update it before merging please? The tests will run with the up to date code.

Copy link
Contributor

@maximearmstrong maximearmstrong left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for this contribution @ed-g!

@maximearmstrong maximearmstrong merged commit 8a64c7a into MobilityData:master May 11, 2022
@@ -23,6 +23,7 @@ dependencies {
implementation 'com.beust:jcommander:1.48'
implementation 'com.google.code.gson:gson:2.8.6'
implementation 'com.github.stefanbirkner:system-lambda:1.2.0'
implementation 'uk.org.webcompere:system-stubs-core:2.0.0'
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for coming in late here, but if this is just used in tests then I believe this should be testImplementation similar to the below line. Otherwise we're going to be packaging this dependency with the release JAR.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like latest version is 2.0.1

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for coming in late here, but if this is just used in tests then I believe this should be testImplementation similar to the below line. Otherwise we're going to be packaging this dependency with the release JAR.

I was not aware of this distinction. I will know for next time :) Everything is fixed in #1140

@@ -23,6 +23,7 @@ dependencies {
implementation 'com.beust:jcommander:1.48'
implementation 'com.google.code.gson:gson:2.8.6'
implementation 'com.github.stefanbirkner:system-lambda:1.2.0'
Copy link
Member

Choose a reason for hiding this comment

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

Can dependency on 'com.github.stefanbirkner:system-lambda:1.2.0' be removed now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, I didn't notice. Good catch.

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.

4 participants