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.

Sign up for GitHub

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

Jump to bottom

feat: Parameterized tests options #2046

Merged
merged 12 commits into from
Jul 2, 2021
Merged

feat: Parameterized tests options #2046

merged 12 commits into from
Jul 2, 2021

Conversation

Sloox
Copy link
Contributor

@Sloox Sloox commented Jun 24, 2021 •
edited
Loading

Fixes #2023

Test Plan

Need to add parameterized test artifacts to flank.

Checklist

  • Documented
  • Unit tested
  • Integration tests updated

@Sloox Sloox added Tiger 🐯 New Option Used to track PR with new configuration option (useful when updating fladle) labels Jun 24, 2021
@Sloox Sloox self-assigned this Jun 24, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jun 24, 2021 •
edited
Loading

Timestamp: 2021-07-01 15:44:33
Buildscan url for ubuntu-workflow run 990653732
https://gradle.com/s/fvcql2ul2j3ro

@bootstraponline bootstraponline changed the title feat: Paramterized tests options feat: Parameterized tests options Jun 28, 2021
@Sloox Sloox removed the Research label Jun 29, 2021
@Sloox Sloox marked this pull request as ready for review June 29, 2021 10:14
@Flank Flank deleted a comment from github-actions bot Jun 29, 2021
@Sloox
Copy link
Contributor Author

Sloox commented Jun 29, 2021

@flank-it

@github-actions
Copy link
Contributor

github-actions bot commented Jun 29, 2021 •
edited
Loading

Integration tests succeed for all OSes ✅
Windows Build scan:
MacOS Build scan: https://gradle.com/s/vq6eapisusguu
Linux Build scan: https://gradle.com/s/jyj7ecoiffc5k
Workflow run https://github.com/Flank/flank/actions/runs/982461748

@pawelpasterz pawelpasterz enabled auto-merge (squash) June 30, 2021 04:14
pawelpasterz
pawelpasterz reviewed Jul 1, 2021
View reviewed changes
Copy link
Contributor

@pawelpasterz pawelpasterz left a comment

Choose a reason for hiding this comment

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

It would be also worth to add possible values of --parameterized-test option in description (AndroidGcloudConfig)

test_runner/src/main/kotlin/ftl/run/platform/android/CreateAndroidTestContext.kt
@@ -119,13 +155,16 @@ internal fun InstrumentationTestContext.getFlankTestMethods(
.filter(testFilter.shouldRun)
.filterNot(parameterizedClasses::belong)
.map(TestMethod::toFlankTestMethod).toList()
.plus(parameterizedClasses.onlyShouldRun(testFilter))
.plus(parameterizedClasses.onlyShouldRun(testFilter, parameterizedTests.shouldIgnore()))
Copy link
Contributor

Choose a reason for hiding this comment

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

#shouldIgnore is a simple method, I think we can return an empty list here instead of inside onlyShouldRun method

Suggested change
.plus(parameterizedClasses.onlyShouldRun(testFilter, parameterizedTests.shouldIgnore()))
.plus(if (parameterizedTests.shouldIgnore()) emptyList() else parameterizedClasses.onlyShouldRun(testFilter))

Not saying it have to look like this, but I'm sure you got the idea.
WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at simply neatness i think having an if statement in a chained set of function calls looks worse than separating it out like is being done for all the other chained function calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it doesn't have to be a chained call. I'm saying that we can avoid passing a boolean parameter that will be used only to decide if we should return an empty list or not. That's all :)

@pawelpasterz
Copy link
Contributor

And I think we could also add this option to additional-app-test-apks

@Sloox Sloox requested a review from pawelpasterz July 1, 2021 12:53
@pawelpasterz
Copy link
Contributor

@flank-it

@github-actions
Copy link
Contributor

github-actions bot commented Jul 2, 2021 •
edited
Loading

Integration tests succeed for all OSes ✅
Windows Build scan:
MacOS Build scan: https://gradle.com/s/palorl7fkfg7s
Linux Build scan: https://gradle.com/s/rrzvjnkhaskve
Workflow run https://github.com/Flank/flank/actions/runs/992844615

@pawelpasterz pawelpasterz merged commit 377c7cf into master Jul 2, 2021
@pawelpasterz pawelpasterz deleted the 2023-new-options-1 branch July 2, 2021 10:48
@github-actions github-actions bot locked and limited conversation to collaborators Jul 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pawelpasterz pawelpasterz pawelpasterz approved these changes

@adamfilipow92 adamfilipow92 adamfilipow92 approved these changes

@jan-goral jan-goral Awaiting requested review from jan-goral

@zuziaka zuziaka Awaiting requested review from zuziaka

@piotradamczyk5 piotradamczyk5 Awaiting requested review from piotradamczyk5

Assignees

@Sloox Sloox

Labels
Feature New Option Used to track PR with new configuration option (useful when updating fladle) P1 Tiger 🐯
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Parameterized Tests: Add new options #1
3 participants
@Sloox @pawelpasterz @adamfilipow92