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

fix: JUnitReport.xml only contained 50 test results #1649

Merged
merged 3 commits into from
Mar 2, 2021
Merged

fix: JUnitReport.xml only contained 50 test results #1649

merged 3 commits into from
Mar 2, 2021

Conversation

dmytrodanylyk
Copy link
Contributor

@dmytrodanylyk dmytrodanylyk commented Mar 1, 2021 •
edited
Loading

Fixes #1634

Test Plan

How do we know the code works?

Existing tests should not be broken

Checklist

  • Documented
  • Unit tested
  • Integration tests updated

@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2021 •
edited
Loading

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

@bootstraponline bootstraponline force-pushed the #1634-junitreport-includes-only-50-tests branch from 70e871e to f69ba8f Compare March 1, 2021 22:57
@dmytrodanylyk
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

artour-bakiev
artour-bakiev reviewed Mar 2, 2021
View reviewed changes
test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt
val testCases = response.testCases.toMutableList()
while (response.nextPageToken != null) {
response = listTestCases(results, response.nextPageToken)
testCases += response.testCases ?: emptyList()
Copy link

Choose a reason for hiding this comment

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

Should we break the loop if an empty list found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure, this code is just copy/paste from a similar code in this file (see listAllEnvironments, listAllSteps)

test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt
@@ -123,7 +124,10 @@ object GcToolResults {
.executeWithRetry()

// Lists Test Cases attached to a Step
fun listTestCases(toolResultsStep: ToolResultsStep): ListTestCasesResponse {
fun listTestCases(
Copy link

Choose a reason for hiding this comment

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

Looks like we should update unit tests as well.

@dmytrodanylyk dmytrodanylyk force-pushed the #1634-junitreport-includes-only-50-tests branch from 8ec4c44 to 4e893f9 Compare March 2, 2021 00:20
github-actions bot added a commit that referenced this pull request Mar 2, 2021
@github-actions
@dmytrodanylyk has signed the CLA from Pull Request #1649
4c185c9
@pawelpasterz
Copy link
Contributor

Does it solve the sharding problem as well?

@dmytrodanylyk
Copy link
Contributor Author

Does it solve the sharding problem as well?

Nope, just JUniteResults.xml fix.

@pawelpasterz pawelpasterz mentioned this pull request Mar 2, 2021
Closed
@piotradamczyk5
Copy link
Contributor

@Mergifyio rebase

@mergify
Copy link

mergify bot commented Mar 2, 2021

Command rebase: action_required

Pull request must be rebased manually.
GitHub App like Mergify are not allowed to rebase pull request where .github/workflows is changed.

@mergify mergify bot merged commit 7880e59 into Flank:master Mar 2, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Mar 2, 2021
mergify bot pushed a commit that referenced this pull request Mar 5, 2021
@pawelpasterz
fix: Fix NPE in GcToolResults (#1662)
fe72d53 
Fixes #1649

PR adds NPE handling when fetching results

## Test Plan
> How do we know the code works?

1. build workflows finish with success
2. IT pass
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@artour-bakiev artour-bakiev artour-bakiev left review comments

@piotradamczyk5 piotradamczyk5 piotradamczyk5 approved these changes

@Sloox Sloox Sloox approved these changes

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

@bootstraponline bootstraponline Awaiting requested review from bootstraponline

@adamfilipow92 adamfilipow92 Awaiting requested review from adamfilipow92

@zuziaka zuziaka Awaiting requested review from zuziaka

@pawelpasterz pawelpasterz Awaiting requested review from pawelpasterz

Assignees

@dmytrodanylyk dmytrodanylyk

Labels
Bug P0 Tiger 🐯
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JUnitReport.xml only includes 50 tests out of 73
5 participants
@dmytrodanylyk @pawelpasterz @piotradamczyk5 @artour-bakiev @Sloox