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

feat: Save am instrument logs to file & handle parsing error. #2049

Merged
merged 6 commits into from
Jun 30, 2021

Conversation

jan-goral
Copy link
Contributor

@jan-goral jan-goral commented Jun 28, 2021

Fixes #2037

Improvements:

  • Save am instrument logs to file in adb_log output directory.
  • Catch am instrument output parsing error and mark affected lines in the error message.
  • Close console connection without waiting for idle, if receive expected results or error.

Test Plan

How do we know the code works?

  • Unit test passes.
  • Running flank corellium test android run creates adb_log inside output directory with dumped am instrument console outputs.
  • If parsing tests results from am instrument log fails the error message in the console will communicate file with lines where the error occurs.

Checklist

  • Documented
  • Unit tested

Catch `am instrument` output parsing error.
If `am instrument` logs paring failed, mark affected lines in error message.
Close console connection right after receiving expected results or error without waiting for idle.
@jan-goral jan-goral force-pushed the 2037_Handle_am_instrument_errors branch from df48115 to 7e66bb9 Compare June 28, 2021 21:07
@jan-goral jan-goral force-pushed the 2037_Handle_am_instrument_errors branch from 7e66bb9 to 0ece007 Compare June 28, 2021 21:54
@jan-goral jan-goral marked this pull request as ready for review June 28, 2021 21:59
@github-actions
Copy link
Contributor

github-actions bot commented Jun 28, 2021

Timestamp: 2021-06-29 16:03:28
Buildscan url for ubuntu-workflow run 983316569
https://gradle.com/s/liu7ls7pa2ojs

maxShardsCount = 1,
)

private val dir = File(args.outputDir)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of File use TemporaryFolder (JUnit). Current implementation does not guarantee clean up after failed tests/exception thrown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of File use TemporaryFolder (JUnit). Current implementation does not guarantee clean up after failed tests/exception thrown

I do not agree. This is the first shot from duckduckgo for the phase JUnit After: https://junit.org/junit4/javadoc/4.12/org/junit/After.html

Copy link
Contributor

Choose a reason for hiding this comment

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

And this is the short from google (I do not use duckduckgo, maybe I should start to do so)
https://junit.org/junit4/javadoc/4.12/org/junit/rules/TemporaryFolder.html

@After may throw an exception itself which will break i.e. recursive deletion.
Not saying your implementation will not work, most likely it will be working fine.
TemporaryFolder is implemented to be used in exactly the same cases we have in this particular test.

Copy link
Contributor Author

@jan-goral jan-goral Jun 29, 2021

Choose a reason for hiding this comment

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

Ok, so according to documentation, the TemporaryFolder is also not guaranteed deletion, similar to @After. Unlike, it will not throw an exception on recursive deletion fail. This is bad because the test should always clean up its own garbages, if it is not possible that means probably some external process is reading the files created during the test. This situation is an edge case that should throw an exception.

import org.junit.Assert
import org.junit.Test

class ExecuteAndroidTestPlanKtTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add unmockkAll() tear down method -- unmocked tests may interact between test suites (we had this issue couple of times in the past)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

"""
Error while parsing results from instance $id.
For details check $logFile lines $lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

Blank line left intentionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup

@pawelpasterz pawelpasterz enabled auto-merge (squash) June 29, 2021 05:22
@jan-goral jan-goral requested a review from pawelpasterz June 29, 2021 15:56
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'm not giving the approval since I do not agree with some decisions in tests (personal preferences). But I don't want to block anything when it should work fine.

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'm not giving the approval since I do not agree with some decisions in tests (personal preferences). But I don't want to block anything when it should work fine.

ah, whatever

@jan-goral
Copy link
Contributor Author

ah, whatever

Thx 😄 👍

@pawelpasterz pawelpasterz merged commit e85039a into master Jun 30, 2021
@pawelpasterz pawelpasterz deleted the 2037_Handle_am_instrument_errors branch June 30, 2021 11:20
@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle 'am instrument' errors
3 participants