-
Notifications
You must be signed in to change notification settings - Fork 158
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
#404: Using documents to avoid storage restrictions #423
#404: Using documents to avoid storage restrictions #423
Conversation
kaspresso/src/main/kotlin/com/kaspersky/kaspresso/files/dirs/DefaultDirsProvider.kt
Outdated
Show resolved
Hide resolved
kaspresso/src/main/kotlin/com/kaspersky/kaspresso/files/dirs/DefaultDirsProvider.kt
Outdated
Show resolved
Hide resolved
kaspresso/src/main/kotlin/com/kaspersky/kaspresso/files/dirs/DefaultDirsProvider.kt
Outdated
Show resolved
Hide resolved
...src/main/kotlin/com/kaspersky/kaspresso/files/resources/impl/DefaultResourceFilesProvider.kt
Outdated
Show resolved
Hide resolved
kaspresso/src/main/kotlin/com/kaspersky/kaspresso/kaspresso/Kaspresso.kt
Outdated
Show resolved
Hide resolved
...kotlin/com/kaspersky/components/alluresupport/interceptors/testrun/MoveReportsInterceptor.kt
Outdated
Show resolved
Hide resolved
* /sdcard/Documents/.../allure-results | ||
* @return allure target directory on sdcard | ||
*/ | ||
private fun moveAllureReportToSdCard(): File { |
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.
nit: maybe it would be better if the moveAllureReportToSdCard() method didn't return anything?
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? Personally I like when file manipulation methods do this like createDirIfNeeded()
does
...kotlin/com/kaspersky/components/alluresupport/interceptors/testrun/MoveReportsInterceptor.kt
Outdated
Show resolved
Hide resolved
.../com/kaspersky/components/alluresupport/interceptors/testrun/AllureTestRunStateHolderTest.kt
Outdated
Show resolved
Hide resolved
Discussed with @Nikitae57 to remove all permission rules in the repository. |
# Conflicts: # tutorial/src/androidTest/kotlin/com/kaspersky/kaspresso/tutorial/kautomator_test/UiSimpleTest.kt # tutorial/src/androidTest/kotlin/com/kaspersky/kaspresso/tutorial/test/SimpleTest.kt
…ure properties to get allure dir name
…ctions' into issue-404/Support_storage_restrictions # Conflicts: # wiki/09_Kaspresso-Allure.md
Co-authored-by: Egor Kurnikov <[email protected]>
…ctions' into issue-404/Support_storage_restrictions
|
||
private var lastTestCaseUuid: String? = null | ||
|
||
override fun onMainSectionStarted(testInfo: TestInfo) { |
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 is it onMainSectionStarted
and not onTestStarted
by the way?
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.
We're starting the video recording in the onTestStarted
, so if there will be failure in before
block the test will be finished without calling onMainSectionStarted
. Thus to save the video we will use the lastTestCaseUuid
from the previous test.
*/ | ||
private fun removeStubs(allureTargetDir: File) { | ||
allureTargetDir.listFiles()?.forEach { | ||
if (it.extension.contains(MP4_EXTENSION, ignoreCase = true)) { |
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.
Maybe we could define the stub more precisely? Not just any mp4 file
override val screenshotsRootDir = File("screenshots") | ||
override val videoRootDir = File("video") | ||
override val viewHierarchy = File("view_hierarchy") | ||
override val allureRootDir = File(PropertiesUtils.resultsDirectoryPath) |
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.
Since the allure.properties
can't be changed in runtime maybe we could cache the PropertiesUtils.resultsDirectoryPath
's result and access the cached value in MoveReportsInterceptor
to not load it on each call?
* @param allureTargetDir allure directory under /sdcard | ||
*/ | ||
private fun saveAttachedVideo(attachedVideo: AttachedVideo, allureTargetDir: File) { | ||
val allureReportFile = allureTargetDir.resolve("${lastTestCaseUuid ?: ""}-result.json") |
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.
Seems like we should use io.qameta.allure.kotlin.AllureConstants.TEST_RESULT_FILE_SUFFIX
here instead.
} | ||
|
||
override fun onTestFinished(testInfo: TestInfo, success: Boolean) { | ||
val allureTargetDir = moveAllureReportToSdCard() |
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.
If the user adds custom interceptors after this one he wouldn't be able to see its results in the report. For example AllureSupportCustomizeTest
will write custom view hierarchy which will not be added to the report.
The construction like Kaspresso.Builder.simple().addAllureSupport().apply { ... }
loses any sense in such a case. So this should be fixed.
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.
First I would suggest to move here that hacky forcing the allure to write down the report to have something like:
override fun onTestFinished(testInfo: TestInfo, success: Boolean) {
forceAllureTestFinished(success)
val allureTargetDir = moveAllureReportToSdCard()
removeStubs(allureTargetDir)
moveAttachedVideosToProperDirectories(allureTargetDir)
cleanUp()
}
fun forceAllureTestFinished(success: Boolean) {
val uuid = Allure.lifecycle.getCurrentTestCase() ?: “”
Allure.lifecycle.updateTestCase {
it.status = if (success) Status.PASSED else Status.FAILED
}
Allure.lifecycle.stopTestCase(uuid)
Allure.lifecycle.writeTestCase(uuid)
}
And then we should guarantee that this MoveReportsInterceptor
is called last no matter how much interceptors the user added by himself. How to achieve this is need to be designed.
val videosDir = dirsProvider.provideNew(rootDirsProvider.videoRootDir) | ||
device.executeShellCommand("rm -rf ${videosDir.absolutePath}") | ||
|
||
dirsProvider.provideNew(rootDirsProvider.screenshotsRootDir).deleteRecursively() |
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.
This would be a behaviour breaking change. Not sure that we have to clear all these dirs in this line and below, previously these files were just copied to the report. I would say we can leave it as it was, but I don't insist, need more opinions here. At least it should be mentioned in the changelog.
…ure results hack refactored
ISSUE-404: added kaspresso runner, added kaspresso run listeners, allure results hack refactored
If someone needs more context, pls take a look at the description and comments here |
import org.junit.Assert.assertFalse | ||
import org.junit.Assert.assertTrue | ||
import org.junit.Rule | ||
import org.junit.Test | ||
import java.io.File |
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 this dependency declaration was moved?
import org.junit.Rule | ||
import org.junit.Test | ||
import java.util.Locale |
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 this dependency declaration was moved?
*/ | ||
// TODO implement allure support sanity test in another way. Current code doesn't fail test if report is invalid | ||
@Ignore("Since we switched to run notifiers this test can no longer be functional") | ||
class AllureSupportSanityTest : TestCase( |
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.
let's move to allure-support module
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.
Moved to a separate folder
/** | ||
* Check that doc loc tests are sane | ||
*/ | ||
class DocLocSanityTest : DocLocScreenshotTestCase( |
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.
let's move to Kaspresso module
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.
Moved to a separate folder
class KaspressoRunner : AndroidJUnitRunner(), KaspressoRunNotifier by KaspressoRunNotifierImpl() { | ||
|
||
override fun onCreate(arguments: Bundle) { | ||
arguments.putArgs("listener", SpyRunListener::class.java.name) |
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.
let's add more comments
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.
Done
import org.junit.runner.notification.Failure | ||
import org.junit.runner.notification.RunListener | ||
|
||
internal class SpyRunListener : RunListener() { |
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.
add Java doc
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.
Done
import org.junit.runner.Result | ||
import org.junit.runner.notification.Failure | ||
|
||
class AllureRunListener : KaspressoLateRunListener { |
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.
make note that this is an allure copy
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.
Left comments
testStarted(cache.firstTestStartedDescription) | ||
} | ||
|
||
class Cache { |
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.
Add some info in comments what is it about. Move to notifier class
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.
Done
|
||
override fun addListener(listener: KaspressoRunListener) { | ||
if (listener is KaspressoLateRunListener) { | ||
listener.lateInit(cache) |
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.
add some info in comments why do we need this method invocation. (Because some listeners are added after tests started and they left call back invocations)
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.
Allure listener and allure runner
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.
Done
|
||
import org.junit.runner.Description | ||
|
||
interface KaspressoLateRunListener : KaspressoRunListener { |
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 it is late? Add some details
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.
Done
…tion to get run notifier from instrumentation
Closes #407, #404, #425 and #418
Also repaired allure video recording