Skip to content

Export To Har#880

Merged
vbuberen merged 20 commits into
ChuckerTeam:developfrom
BluestormDNA:feature/exportToHar
Sep 29, 2022
Merged

Export To Har#880
vbuberen merged 20 commits into
ChuckerTeam:developfrom
BluestormDNA:feature/exportToHar

Conversation

@BluestormDNA

@BluestormDNA BluestormDNA commented Sep 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Edited the export function that writes transaction data to a file and
returns a URI to access that file to also support .HAR format as a parameter.

📷 Screenshots

Internal Changes. The sample app layout has been updated to reflect the functionality: "Export to Har" button.

Screenshot_1663334392
Screenshot_1663334610

📄 Context

Closes #871
It adds support for writeTransactions to also export on .HAR format

📝 Changes

Pending.

📎 Related PR

None

🚫 Breaking

None

🛠️ How to test

It can be tested from the UI just adding the ExportFormat

⏱️ Next steps

No.

@cortinico cortinico marked this pull request as draft September 10, 2022 15:43
@cortinico

cortinico commented Sep 10, 2022

Copy link
Copy Markdown
Member

Converting this to Draft. Feel free to ping me when you're done and need a review

@BluestormDNA BluestormDNA mentioned this pull request Sep 10, 2022
Closed
2 tasks
@BluestormDNA

BluestormDNA commented Sep 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Go ahead @cortinico
As I commented on #871:
Please be aware that the @SuppressLint("VisibleForTests") is there because HarUtils only exposes a suspend fun and i needed the underlying blocking method.
I'll await comments.

cortinico
cortinico requested changes Sep 10, 2022
View reviewed changes

@cortinico cortinico left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for sneding this over.

You're missing several critical changes:

  1. Please fill in the pull request template.
  2. Update the library-no-op variant of the library as well.
  3. Update the CHANGELOG with this change
  4. Add tests for this change.

Comment thread library/src/main/kotlin/com/chuckerteam/chucker/api/ChuckerCollector.kt Outdated
Comment thread library/src/main/kotlin/com/chuckerteam/chucker/internal/support/ExportFormat.kt Outdated
Comment thread library/src/main/kotlin/com/chuckerteam/chucker/api/ChuckerCollector.kt Outdated
Comment thread library/src/main/kotlin/com/chuckerteam/chucker/api/ChuckerCollector.kt Outdated
@BluestormDNA

BluestormDNA commented Sep 10, 2022

Copy link
Copy Markdown
Contributor Author

About tests: the writeTransactionsoriginal PR added a button to the Sample App to export.
Should i add another button below? or maybe put a radio button to select?

@cortinico cortinico marked this pull request as ready for review September 10, 2022 16:45
cortinico
cortinico reviewed Sep 10, 2022
View reviewed changes
Comment thread library-no-op/src/main/kotlin/com/chuckerteam/chucker/api/ChuckerCollector.kt Outdated
Comment thread library-no-op/src/main/kotlin/com/chuckerteam/chucker/api/ExportFormat.kt Outdated
Comment thread library/src/main/kotlin/com/chuckerteam/chucker/api/ExportFormat.kt Outdated
@cortinico

cortinico commented Sep 10, 2022

Copy link
Copy Markdown
Member

Should i add another button below? or maybe put a radio button to select?

Either two button side by side or a radio button work 👍

@vbuberen

vbuberen commented Sep 13, 2022

Copy link
Copy Markdown
Collaborator

Nothing to see, internal changes.

Seems like after adding a button there is now something to see. Would be good to have some screenshots.

cortinico
cortinico approved these changes Sep 13, 2022
View reviewed changes
Comment thread sample/src/main/res/layout/activity_main_sample.xml
cortinico
cortinico approved these changes Sep 17, 2022
View reviewed changes

@cortinico cortinico left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome stuff 👍 Thanks for taking a stance at this

@cortinico

cortinico commented Sep 29, 2022

Copy link
Copy Markdown
Member

@vbuberen can we merge this?

@vbuberen

vbuberen commented Sep 29, 2022

Copy link
Copy Markdown
Collaborator

Sure, looks good to me.

@vbuberen vbuberen merged commit 6fa5be2 into ChuckerTeam:develop Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cortinico cortinico cortinico approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add export function to API (.har file)

3 participants

@BluestormDNA @cortinico @vbuberen