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.

Sign up for GitHub

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

Jump to bottom

Adding future-proof error message matcher for upload hash validation #1155

Merged
merged 4 commits into from
Sep 13, 2022
Merged

Adding future-proof error message matcher for upload hash validation #1155

merged 4 commits into from
Sep 13, 2022

Conversation

MewX
Copy link
Contributor

@MewX MewX commented Sep 1, 2022

No description provided.

jzacsh
jzacsh requested changes Sep 6, 2022
View reviewed changes
Copy link
Collaborator

@jzacsh jzacsh left a comment

Choose a reason for hiding this comment

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

could you add test coverage for this new edge case in to your existing tests? GooglePhotosImporterTest.java

.../src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosInterface.java Show resolved Hide resolved
.../src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosInterface.java Show resolved Hide resolved
jzacsh
jzacsh requested changes Sep 6, 2022
View reviewed changes
Copy link
Collaborator

@jzacsh jzacsh left a comment

Choose a reason for hiding this comment

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

could you add test coverage for this new edge case in to your existing tests? GooglePhotosImporterTest.java

@MewX
Copy link
Contributor Author

MewX commented Sep 8, 2022

could you add test coverage for this new edge case in to your existing tests? GooglePhotosImporterTest.java

Currently GooglePhotosImporterTest only receives the mocked UploadErrorException signal, which has been tested.

This new error string comes from network request, and should be in GooglePhotosInterfaceTest. However, it doesn't exist yet. Due to time limit, can I skip creating this test from scratch 😂 Thanks!

@jzacsh jzacsh mentioned this pull request Sep 8, 2022
Open
@jzacsh
Copy link
Collaborator

jzacsh commented Sep 8, 2022

could you add test coverage for this new edge case in to your existing tests? GooglePhotosImporterTest.java

Currently GooglePhotosImporterTest only receives the mocked UploadErrorException signal, which has been tested.

This new error string comes from network request, and should be in GooglePhotosInterfaceTest. However, it doesn't exist yet. Due to time limit, can I skip creating this test from scratch joy Thanks!

ah that's unfortunate - I've filed #1158 - thanks for pointing this out!

@jzacsh jzacsh merged commit 34a2280 into dtinit:master Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzacsh jzacsh jzacsh 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.
2 participants
@MewX @jzacsh