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

Add export function to API #784

Merged
merged 8 commits into from
Jul 30, 2022
Merged

Conversation

jd565
Copy link
Contributor

@jd565 jd565 commented Mar 12, 2022

Add an export function that writes transaction data to a file and
returns a URI to access that file. The output format is the same as when
exporting from the chucker UI, but can be triggered from code.

📷 Screenshots

All internal changes - nothing different in UI

📄 Context

Adds functionality to export transactions to a file on the API. Related issue

📝 Changes

Updated the Chucker API object to add in the new functionality. I've not made this a suspending function to maintain interop with java, so have just left a comment that this needs to be called on a background thread.

I added a button to the sample app (I had to comment out the strict mode checking otherwise the sample app closed immediately) which calls the new method - I'm not sure if this should be left in the sample long term so happy to remove it if it doesn't make sense.

📎 Related PR

🚫 Breaking

No

🛠️ How to test

Added a button to the sample app to export to a file then pulled the file and verified the contents.

⏱️ Next steps

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 🙏 Left several comments that needs to be addressed before we can proceed

CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines 43 to 45
Thread {
Chucker.writeTransactions(this@MainActivity, 1, null)
}.start()
Copy link
Member

Choose a reason for hiding this comment

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

Please don't start a thread explicity here. We use coroutines to do any async work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have updated to use coroutines, but have had to add coroutines and activity-ktx dependencies to the sample gradle project as they weren't there before

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I realized this is a side effect 🤔 I'll be fine with it. I'm unsure what's the other maintainers take on this (specifically @MiSikora )

@cortinico
Copy link
Member

I've not made this a suspending function to maintain interop with java, so have just left a comment that this needs to be called on a background thread.

Just noticed this. I would say that we can safely make this a suspending function at this stage.

Moreover, tests are missing. Could you add some of them?

@jd565
Copy link
Contributor Author

jd565 commented Mar 28, 2022

I think I have covered all your comments. Could you give some more information about what functions you are expecting to be tested?

The old Sharable.shareAsFile function has no tests on it, so I am not sure if you want that to be tested? The overall function aswell in ChuckerCollector I am not sure if you want a test on but I am not 100% sure how to achieve this without mocking everything and leaving a not very useful test.

@jd565
Copy link
Contributor Author

jd565 commented Mar 28, 2022

@cortinico Thanks for looking over it. I think I have addressed all the comments above, but I'm not 100% sure about the testing side of it. Could you clarify what parts you think should have unit test coverage and I can look into it?

@jd565
Copy link
Contributor Author

jd565 commented Apr 9, 2022

@cortinico Can you give any advice about the above?

@cortinico
Copy link
Member

@cortinico Can you give any advice about the above?

Sorry this fell off @jd565 my bad. I was quite busy recently.
I've pushed fc561b1 on top of your branch.

I believe the best approach here would be to make writeTransactions blocking, and let the user orchestrate how to execute it. Sorry for the back and forth :/

I'd like to get your opinion on the last commit. If you're fine with it, we can merge it.
Also getting @vbuberen pov here would be great 👍

Copy link
Collaborator

@vbuberen vbuberen left a comment

Choose a reason for hiding this comment

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

The approach looks fine to me.

Have left a few comments about strange new imports.

Also, I feel that we need to mention this feature in our Readme file as well.

@cortinico
Copy link
Member

Have left a few comments about strange new imports.

It seems like we haven't had the detekt rule enabled about unused imports. I've enabled it and fixed other issues in the codebase.

@jd565
Copy link
Contributor Author

jd565 commented Jul 5, 2022

@cortinico I also forgot about this :/ Your changes look fine and if you are happy to merge then I'm happy to.

@BluestormDNA
Copy link
Contributor

@jd565 @cortinico cound it be possible to add HAR file export too?

jd565 and others added 8 commits July 30, 2022 18:03
Add an export function that writes transaction data to a file and
returns a URI to access that file. The output format is the same as when
exporting from the chucker UI, but can be triggered from code.
@cortinico
Copy link
Member

@cortinico I also forgot about this :/ Your changes look fine and if you are happy to merge then I'm happy to.

Yup I'm merging this one 👍 Thank you again @jd565

@jd565 @cortinico cound it be possible to add HAR file export too?

@BluestormDNA please open a separate issue

@cortinico cortinico merged commit 926fdbb into ChuckerTeam:develop Jul 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants