-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: Add logic to verify xml results #1362
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1362 +/- ##
============================================
+ Coverage 77.56% 77.58% +0.02%
Complexity 717 717
============================================
Files 248 248
Lines 4788 4788
Branches 915 915
============================================
+ Hits 3714 3715 +1
Misses 572 572
+ Partials 502 501 -1 |
Timestamp: 2020-12-11 12:58:58 |
@@ -29,6 +30,7 @@ class AllTestFilteredIT { | |||
} | |||
|
|||
@Test | |||
@Ignore("Should be fixed in https://github.com/Flank/flank/issues/1388") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we ignore this test for all OS ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only for ios, android works but output could change after #1388
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this will be picked up and removed in that ticket. Can we add a // TODO with a ticket number
@@ -1,3 +1,6 @@ | |||
gcloud: | |||
test: ../test_runner/src/test/kotlin/ftl/fixtures/tmp/earlgrey_example.zip | |||
xctestrun-file: ../test_runner/src/test/kotlin/ftl/fixtures/tmp/EarlGreyExampleSwiftTests_iphoneos13.4-arm64e.xctestrun | |||
device: | |||
- model: iphone8 | |||
version: 13.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we force to use this device?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use the default device. Thanks! Changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks ok, but IT tests are failing on my local run. I guess it's related to my local configuration rather than the branch.
https://gist.github.com/jan-gogo/5b52b7a2c6fc0d085175bfa0b2b749c7
@@ -65,16 +65,5 @@ RunTests | |||
Saved 1 shards to android_shards.json | |||
Uploading app-debug.apk \.* | |||
Uploading app-single-success-debug-androidTest.apk \.* | |||
1 test / 1 shard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why those lines are removed?
I launched it locally and the timeout case failed because of it 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locally I get the output without removed lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this test by using test
and apk
from gcs. Using local files can produce different outputs related to file uploading speed.
|
||
fun TestSuites.assertCountOfSkippedTests(expectedCount: Int) = assertEquals(expectedCount, testSuites.sumBy { it.skipped }) | ||
|
||
fun TestSuites.assertCountOfFailedTests(expectedCount: Int) = assertEquals(expectedCount, testSuites.count { it.failures > 0 }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does detekt not pick this up as too long? can you run it?
@@ -29,6 +30,7 @@ class AllTestFilteredIT { | |||
} | |||
|
|||
@Test | |||
@Ignore("Should be fixed in https://github.com/Flank/flank/issues/1388") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this will be picked up and removed in that ticket. Can we add a // TODO with a ticket number
import utils.testResults.TestSuites | ||
import java.io.File | ||
|
||
fun File.loadAsTestSuite(): TestSuites = XmlMapper().registerModule(KotlinModule()).readValue(this, TestSuites::class.java) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again with long lines and detekt failing. Do you have githooks installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have installed it.
Strange. To be sure I run detekt and check and both pass.
import java.io.File | ||
import java.nio.file.Paths | ||
|
||
fun String.findTestDirectoryFromOutput() = "results-dir:\\s.*\\s".toRegex().find(this)?.value.orEmpty().trim().replace("results-dir: ", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to request changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One test was failed on my local run
https://gist.github.com/jan-gogo/9e2f4d96f05f1efc3a1831bd1709d95e
[EDIT]
...due to Insufficient test quota
Fixes #1315
Test Plan
./gradlew integrationTests
Description
Flank should also verify XML results for android and ios integration tests.
This ticket should be merged after #1316 when we update all tests.
filter all tests - ios
is disabled and should be updated in #1388Checklist