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

techdebt: backfill missing unit tests for GooglePhotosInterface.java #1158

Open
jzacsh opened this issue Sep 8, 2022 · 12 comments
Open

techdebt: backfill missing unit tests for GooglePhotosInterface.java #1158

jzacsh opened this issue Sep 8, 2022 · 12 comments
Labels

Comments

@jzacsh
Copy link
Collaborator

jzacsh commented Sep 8, 2022

@MewX ran into this at #1155 (comment)

@mahikaajain
Copy link

Hey, is this issue still open? If yes, can I work on this?

@anand007s
Copy link

please assign it to me...

@FilippoULIVO
Copy link

FilippoULIVO commented Jun 15, 2023

Hey, I noticed that most methods on the class depend on private method makeGetRequest in order to mock it I would need to make it visible but I don't want to break encapsulation. I noticed there is also a makePostRequest which is public and is used for mocked responses in GooglePhotosImporterTest. If it's okay to change the access modifier on the method I can complete unittesting the methods dependent on makeGetRequest.

@jzacsh

@MewX
Copy link
Contributor

MewX commented Jun 16, 2023

Would package-private sufficient for the tests? If so, usually you could mark these methods package-private and annotate them with @VisibleForTesting.

@FilippoULIVO
Copy link

yes ok it works. currentlyt working on this

@FilippoULIVO
Copy link

FilippoULIVO commented Jun 22, 2023

Hey ,sorry for the noob question, I can't find this info.
I'm trying to make a first commit with most of the test cases but I'm not able to push to origin, I'm trying git push -u origin test-importer-interface and receiving 403 error with message remote: Permission to google/data-transfer-project.git denied to FilippoULIVO I just submitted my CLA agrrement, but i'm not sure it's related to that. could you help?

wanted to make a first commits also to check if I could pass all the different style checks

@FilippoULIVO
Copy link

I completed the test coverage for the class.
Btw @MewX as I was sayngmakePostRequest is public, I didn't write test for it because I belive it should be made package-private just as the makeGetRequest ,having it public defeats the purpose of the interface, from IntelliJ I can see it's only used in tests as mock.
I would need some guidance on the Pull request process for submitting my contribute

@jzacsh
Copy link
Collaborator Author

jzacsh commented Jun 23, 2023

I'm trying to make a first commit with most of the test cases but I'm not able to push to origin, I'm trying git push -u origin test-importer-interface and receiving 403 error with message remote: Permission to google/data-transfer-project.git denied to FilippoULIVO

I believe that's because you're accidentally trying to develop directly on this repo, but you instead want to maintain your branch on your own repo (both in draft state and when you're ready to open a PR for it). You can get your own repo by clicking the "fork" button (or visiting https://github.com/google/data-transfer-project/fork).

maybe-helpful guessing on my part here, but please read the docs and run at your own risk (backup your work, etc)

Since you already have a working copy of the repo in your terminal, you can tell that copy tell about your new fork by adding a new git "remote" like so:

$ git remote -v # pay attention to the output from this command; note how it will change later (you'll probably only have "origin" lines right now)...
$ git remote add fork git@github.com:FilippoULIVO/data-transfer-project.git
$ git remote -v # now "fork" should be listed to

I'm guessing there that you have SSH configured, but if not you may want to set-url to HTTPS instead before running this command:

$ git push fork test-importer-interface

Once that's all done your branch "test-importer-interface" will be visible in the github UI under your account (from which a PR can be opened when you're ready)

@FilippoULIVO
Copy link

Please review?

@FilippoULIVO
Copy link

Hey, if this pull request is okay I'm happy to keep contributing to the project @jzacsh

@FilippoULIVO
Copy link

sorry for the repeated trial and error. I don't understand if my pull request is well formulated or if I'm doing something wrong

@FilippoULIVO
Copy link

hey @jzacsh @MewX is this issue still relevant? could you review my merge request please? thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants