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

fix: NPE on dumpShards #1612

Merged
merged 6 commits into from
Feb 18, 2021
Merged

fix: NPE on dumpShards #1612

merged 6 commits into from
Feb 18, 2021

Conversation

adamfilipow92
Copy link
Contributor

Fixes #1601

Test Plan

How do we know the code works?

When use --dump-shards, flank should create dump shard file without errors.

Checklist

  • Unit tested
  • Integration tests updated

@github-actions
Copy link
Contributor

github-actions bot commented Feb 16, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@github-actions
Copy link
Contributor

github-actions bot commented Feb 16, 2021

Timestamp: 2021-02-17 23:35:14
Buildscan url for ubuntu-workflow run 576464689
https://gradle.com/s/7pqjpmi7yxbdw

@adamfilipow92 adamfilipow92 marked this pull request as ready for review February 16, 2021 17:30
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.

I think it would be great to cover more cases within tests. For JSON verification, I think we should verify if ignored tests are present in the right place but also if the number of shards is correct. We should also check if the total number of tests is correct

@adamfilipow92 adamfilipow92 marked this pull request as draft February 17, 2021 08:19
Copy link
Contributor

@Sloox Sloox left a comment

Choose a reason for hiding this comment

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

Has it been tested on Windows ;)

@adamfilipow92
Copy link
Contributor Author

Has it been tested on Windows ;)

Tested and works :)

@adamfilipow92 adamfilipow92 self-assigned this Feb 17, 2021
@adamfilipow92 adamfilipow92 marked this pull request as ready for review February 17, 2021 23:54
)

Truth.assertThat(matrix.junitIgnored.count()).isEqualTo(4)
Truth.assertThat(matrix.junitIgnored).containsNoDuplicates()
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea! 👍

Comment on lines +39 to +44
Truth.assertThat(matrix.shards.values.flatten()).containsAll(
"class com.example.test_app.parametrized.EspressoParametrizedClassParameterizedNamed",
"class com.example.test_app.parametrized.EspressoParametrizedClassTestParameterized",
"class com.example.test_app.ParameterizedTest",
"class com.example.test_app.parametrized.EspressoParametrizedMethodTestJUnitParamsRunner",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the test checks if there are tests inside shards 👍 We could make one step further and check if there are all tests or we can check just the number. (test count does not change and we check for duplication in another line)

@Sloox Sloox self-requested a review February 18, 2021 11:28
Copy link
Contributor

@Sloox Sloox left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@mergify mergify bot merged commit 68166db into master Feb 18, 2021
@mergify mergify bot deleted the 1601-npe-on-dump-shards branch February 18, 2021 11:29
@github-actions github-actions bot locked and limited conversation to collaborators Feb 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NPE on dump shards
3 participants