Fix unit tests for pull request #5262

[nicolas-raoul]

#5262 introduces a useful feature, but makes two unit tests fail.

The task here is to fix these unit tests. Any volunteer? :-)

[axelthepony27]

Hello @nicolas-raoul, I'm new to the Open Source community, and would like to take this as one of my first issues. Can I take over this?

[nicolas-raoul]

@axelthepony27 It is yours, thanks! Please let us know about your progress every few days. :-)

[TomerPacific]

Hi @nicolas-raoul, I'd like to help out if @axelthepony27 is no longer actively working on this.

[nicolas-raoul]

@axelthepony27 If you have some code for this would you mind sending a draft pull request?
If not, I will assign @TomerPacific to it in 24 hours. Thanks for your understanding! 🙂

[TomerPacific]

@nicolas-raoul - Just to make sure, since the PR in question has not been merged yet, I am assuming it is needed to branch out of the PR's branch?

[nicolas-raoul]

Yes exactly! 🙂

[TomerPacific]

@nicolas-raoul - Running the unit tests, I see the following:

• The two unit tests that are failing are:
• testOnClickModifyLocation
• testOnStyleLoaded

These tests are found inside the file LocationPickerActivityUnitTests

When debugging these tests, I see:

• removeLocationButton from LocationPickerActivity is null
• Seeing inside ByteBuddyAgent that when the codeSource tries to use getLocation is null and this section of code throws an exception (don't know how relevant this is):

@SuppressFBWarnings(value = "REC_CATCH_EXCEPTION", justification = "Exception should not be rethrown but trigger a fallback")
    private static File trySelfResolve() {
        try {
            ProtectionDomain protectionDomain = Attacher.class.getProtectionDomain();
            if (protectionDomain == null) {
                return CANNOT_SELF_RESOLVE;
            }
            CodeSource codeSource = protectionDomain.getCodeSource();
            if (codeSource == null) {
                return CANNOT_SELF_RESOLVE;
            }
            URL location = codeSource.getLocation();
            if (!location.getProtocol().equals(FILE_PROTOCOL)) {
                return CANNOT_SELF_RESOLVE;
            }
            try {
                return new File(location.toURI());
            } catch (URISyntaxException ignored) {
                return new File(location.getPath());
            }
        } catch (Exception ignored) {
            return CANNOT_SELF_RESOLVE;
        }

Apart from what is written in the PR, can you elaborate on what was done in it and what the unit tests themselves did before they failed?

[TomerPacific]

@nicolas-raoul - Adding null checks in LocationPickerActivity around removeLocationButton makes the tests pass. So I am tending to believe that the button does not show up as it used to show up previously.
I looked at the PR and the Issue and I want to replicate the user flow.
I tried searching for how to do this through the repository, but could not find anything substantial.
Could you detail what steps are necessary to do this?

I am assuming there is a test user account I can use?

[nicolas-raoul]

@TomerPacific

There is unfortunately no test user account, you will have to create one (free).

Here is the user workflow:

1. Open the app
2. Log in
3. Tap the + sign
4. Tap the gallery icon
5. Select any picture (it will not get actually uploaded)
6. On the caption/description activity, tap the map icon
7. Tap Edit location
8. Tap Remove location
9. In the popup tap Remove location

[TomerPacific]

Thanks @nicolas-raoul.
I have followed the steps you detailed and I have to mention that the edit location and remove location buttons do not appear until you choose a location.

Could that be what is causing the tests to fail?

I assume that previously they might have been there(?).

If this is the case, what would you like done here?

Below is a gif I took that demonstrates the user flow you described above.

[nicolas-raoul]

Could you please try again with a picture that has a location in its EXIF? The file you chose does not have a location.

[TomerPacific]

@nicolas-raoul , tried with several images that have their location in their EXIF, but still saw the same flow.

An example is the image with the dogs used from here.

If you have an image you would like me to use, let me know.

[nicolas-raoul]

@TomerPacific Would you mind sending me an email? I will reply with a link to a Google Drive containing such pictures. My email address is visible at https://nicolasraoul.github.io
Thanks!

[TomerPacific]

@nicolas-raoul - Sent you an email

[nicolas-raoul]

@TomerPacific I just added you to the Drive, thanks! https://drive.google.com/corp/drive/folders/1tXJed0djQUK9V2G7cX7AIMSb6ckwEOzY

[TomerPacific]

@nicolas-raoul - Using one the pictures from the drive link, I am able to see that when you open the location it shows the edit location and remove location buttons.

Question is, why when the unit test testOnStyleLoaded is run, it cannot find the removeLocation button.

Could it be that the test is using an image without a location?

[nicolas-raoul]

Yes, you're on a good lead! Would you mind finding and sharing what image is used by the unit test?

[TomerPacific]

@nicolas-raoul - Looking inside the unit tests, I see a folder under resources called ImageTest.

But I don't see them being used anywhere in the project.

Any idea how to proceed here?

[nicolas-raoul]

When you run that unit test in debug mode, where do you see the variables coming from? You may need to study the unit test and mock frameworks being used.

[TomerPacific]

@nicolas-raoul - I have found the problem.
In the test class, a mock was not added for the removeLocationButton. Adding it makes the tests pass.

The problem is, since I used the fork of the user who worked on that feature, I cannot create a PR directly for the repository.

Your advice on how to proceed here will be greatly appreciated.

[nicolas-raoul]

Wonderful!

No problem, either create the pull request there, or create a different branch from the main repo then pull from that person's branch and apply your changes before sending us a pull request. 🙂

[TomerPacific]

@nicolas-raoul - Opened a PR.

[ShashwatKedia]

@nicolas-raoul I think this issue could be closed as #5262 is also closed, and no more unit tests related to that PR or issue is failing.

[nicolas-raoul]

@ShashwatKedia Indeed thanks! :-)