Fix #22: Add Gravatar utility functions #29

maxme · 2024-01-19T09:50:25Z

Fix #22 and fix #11 (by adding some tests)

hamorillo · 2024-01-19T12:25:26Z

.editorconfig

+ktlint_function_naming_ignore_when_annotated_with = Composable
+max_line_length=120


hamorillo · 2024-01-19T12:37:50Z

.gitignore

+# IntelliJ/Android Studio exceptions
+!/.idea/codeStyles/
+!/.idea/fileTemplates/
+!/.idea/inspectionProfiles/
+!/.idea/scopes/
+!/.idea/codeStyleSettings.xml
+!/.idea/encodings.xml
+!/.idea/copyright/
+# Enforce plugins
+!/.idea/externalDependencies.xml
+# Checkstyle configuration
+!/.idea/checkstyle-idea.xml
+


❓Why do we not want to track those files? Some of them allow us to share some code styles for the Android Studio autoformatting. The code will be validated by Ktlint, but I think some of them could be useful. I usually use gitignore.io to see what they suggest. In our case this

Note that we want to track those above ^, but I don't I want to track everything for .idea/ (eg. .idea/deploymentTargetDropDown.xml isn't useful IMO because it changes depending on the tests you run in AS)

I took it wpandroid, I haven't checked everything, but I'm fine using another list.

🤦🏼‍♂️ My bad, I didn't notice the !... Sorry.

hamorillo · 2024-01-19T13:02:01Z

gravatar/src/androidTest/java/com/gravatar/GravatarUtilsInstrumentedTest.kt

+import org.junit.runner.RunWith
+
+@RunWith(AndroidJUnit4::class)
+class GravatarUtilsInstrumentedTest {


⚠️ We need to add these tests as InstrumentedTest because we internally use android.net.Uri but all of them look like a unit test to me. Should we try to add Robolectric and/or mock the Uri.Builder? We don't have DI yet, which complicates things; maybe we should dedicate some time to it?

We don't need to take care of this right now. We can merge and keep working on improving our testing possibilities.

Yeah I was thinking of add Robolectric, but it seemed like an overkill for this. We'll likely have connected tests anyway so why not having them here to test the full path.

I don't think we'll need several connected tests, at least for the non-UI part.
My main concern is about the execution time. Now, it is insignificant, but if we implement the majority of the tests as instrumented tests, the execution time will increase rapidly.

But as I mentioned, we can iterate. 🙂

Good point about speed, I removed connected tests and I setup up Robolectric tests in 3d20326

hamorillo · 2024-01-19T13:05:28Z

gravatar/src/main/java/com/gravatar/GravatarUtils.kt

+    return emailAddressToGravatarUri(email, defaultAvatarImage, size).toString()
+}
+
+fun emailAddressToGravatarUri(


❓Could it make sense to trim the email here, too, and verify it's not empty? Verify that it's a well-formatted e-mail could be going too far?

I doubt we want to go too far here, we can check for empty string, but we shouldn't check if the e-mail address is valid.

gravatar/src/test/java/com/gravatar/GravatarUtilsUnitTest.kt

maxme · 2024-01-19T14:12:05Z

Note this is still a draft PR (but we can't use them in free private repo), I opened it to test github actions.

hamorillo

Note this is still a draft PR (but we can't use them in free private repo)

TIL - Maybe we can add the title DRAFT - PR Title or create a draft label. Wdyt?

Sorry if I was too fast with my comment, I thought the PR was ready and you were waiting the review. Let me know when is ready.

hamorillo · 2024-01-19T14:18:09Z

.gitignore

+# IntelliJ/Android Studio exceptions
+!/.idea/codeStyles/
+!/.idea/fileTemplates/
+!/.idea/inspectionProfiles/
+!/.idea/scopes/
+!/.idea/codeStyleSettings.xml
+!/.idea/encodings.xml
+!/.idea/copyright/
+# Enforce plugins
+!/.idea/externalDependencies.xml
+# Checkstyle configuration
+!/.idea/checkstyle-idea.xml
+


🤦🏼‍♂️ My bad, I didn't notice the !... Sorry.

hamorillo · 2024-01-19T14:22:29Z

gravatar/src/androidTest/java/com/gravatar/GravatarUtilsInstrumentedTest.kt

+import org.junit.runner.RunWith
+
+@RunWith(AndroidJUnit4::class)
+class GravatarUtilsInstrumentedTest {


I don't think we'll need several connected tests, at least for the non-UI part.
My main concern is about the execution time. Now, it is insignificant, but if we implement the majority of the tests as instrumented tests, the execution time will increase rapidly.

But as I mentioned, we can iterate. 🙂

maxme · 2024-01-19T15:15:19Z

Sorry if I was too fast with my comment, I thought the PR was ready and you were waiting the review. Let me know when is ready.

No worries, that's a good pre-review :).

Adding Draft: .... to the title is a good idea. I'll ping you when this one is ready for another pass.

maxme · 2024-01-23T11:46:13Z

I wanted to remove the GravatarUtils.java file from WordPress-Utils before opening this PR, but dependency management is getting more complicated that I thought.

Note: if we want to add connected tests back to CI, we can revert this commit b2de8a5

@hamorillo can you do another pass on this one?

hamorillo

🚀 LGTM!

maxme added 5 commits January 17, 2024 16:17

First version of Gravatar utility functions

cd6ca3e

Merge branch 'trunk' into maxime/22-gravatar-utility-functions

f868a81

Set the same line length as default detekt config

96fe59f

Create a set of unit and connected tests for the Gravatar Utils methods

067058c

new test for the Uri method

0512357

maxme force-pushed the maxime/22-gravatar-utility-functions branch from b372467 to b95b76e Compare January 19, 2024 13:03

hamorillo reviewed Jan 19, 2024

View reviewed changes

maxme force-pushed the maxime/22-gravatar-utility-functions branch 2 times, most recently from 432fed4 to 76c8f83 Compare January 19, 2024 14:24

hamorillo reviewed Jan 19, 2024

View reviewed changes

maxme force-pushed the maxime/22-gravatar-utility-functions branch from 76c8f83 to 653364b Compare January 19, 2024 14:43

Add tests and connect tests to github actions

9953113

maxme force-pushed the maxime/22-gravatar-utility-functions branch from 653364b to 9953113 Compare January 19, 2024 14:56

Add Robolectric and move connected unit tests to unit tests

3d20326

maxme changed the title ~~Fix #22: Add Gravatar utility functions~~ Draft: Fix #22: Add Gravatar utility functions Jan 19, 2024

maxme added 3 commits January 19, 2024 16:17

Remove connected tests from the README and the github workflow

b2de8a5

Move constants to their own file

79beb10

Add a 30mins timeout to CI actions

f814ba2

maxme changed the title ~~Draft: Fix #22: Add Gravatar utility functions~~ Fix #22: Add Gravatar utility functions Jan 23, 2024

hamorillo approved these changes Jan 23, 2024

View reviewed changes

maxme merged commit b7c3177 into trunk Jan 23, 2024
1 check failed

maxme deleted the maxime/22-gravatar-utility-functions branch January 23, 2024 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #22: Add Gravatar utility functions #29

Fix #22: Add Gravatar utility functions #29

maxme commented Jan 19, 2024

hamorillo Jan 19, 2024

hamorillo Jan 19, 2024

maxme Jan 19, 2024 •

edited

Loading

hamorillo Jan 19, 2024

hamorillo Jan 19, 2024

maxme Jan 19, 2024

hamorillo Jan 19, 2024

maxme Jan 19, 2024 •

edited

Loading

hamorillo Jan 19, 2024

maxme Jan 19, 2024

maxme commented Jan 19, 2024

hamorillo left a comment

hamorillo Jan 19, 2024

hamorillo Jan 19, 2024

maxme commented Jan 19, 2024

maxme commented Jan 23, 2024 •

edited

Loading

hamorillo left a comment

		ktlint_function_naming_ignore_when_annotated_with = Composable
		max_line_length=120

Fix #22: Add Gravatar utility functions #29

Fix #22: Add Gravatar utility functions #29

Conversation

maxme commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxme Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxme Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxme commented Jan 19, 2024

hamorillo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxme commented Jan 19, 2024

maxme commented Jan 23, 2024 • edited Loading

hamorillo left a comment

Choose a reason for hiding this comment

maxme Jan 19, 2024 •

edited

Loading

maxme Jan 19, 2024 •

edited

Loading

maxme commented Jan 23, 2024 •

edited

Loading