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

Reworked FieldListUpdateTest #6400

Merged
merged 24 commits into from
Oct 31, 2024
Merged

Reworked FieldListUpdateTest #6400

merged 24 commits into from
Oct 31, 2024

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Sep 6, 2024

Closes #6143

Why is this the best possible solution? Were any other approaches considered?

As outlined in the issue, the primary objectives were to refactor the entire test class to utilize FormEntryActivityTestRule and to update the recently added test (changeInValueUsedToDetermineIfAQuestionIsRequired_ShouldUpdateTheRelatedRequiredQuestion) to use its own form. I also took the opportunity to add some further refactoring to enhance the tests.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

It doesn't require testing.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
  • added a comment above any new strings describing it for translators
  • added any new strings with date formatting to DateFormatsTest
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@grzesiek2010 grzesiek2010 marked this pull request as ready for review September 6, 2024 12:24
@grzesiek2010 grzesiek2010 requested a review from seadowg September 6, 2024 12:24
@@ -84,38 +85,44 @@ class FieldListUpdateTest {

@Test
fun changeInValueUsedInLabel_ShouldChangeLabelText() {
val name = UUID.randomUUID().toString()
Copy link
Member

Choose a reason for hiding this comment

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

What's the advantage of using random answers here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Using a constant here could potentially make the tests less reliable, as the same constant might be used in the logic, causing the test to always pass. However, the main reason I kept the random names was to avoid making significant changes to the tests.

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd prefer leaving them in. I totally understand that there's a benefit to random input data, but I don't think it's huge here, and it makes the tests less readable (to me anyway) and also makes me worry about why UUIDs are being used.

Down the line we could experiment with helpers to generate random data like names and numbers etc that would fit better in tests. That's a pretty common approach I've used on other projects.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

…uired_ShouldUpdateTheRelatedRequiredQuestion test
Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

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

@grzesiek2010 grzesiek2010 requested a review from seadowg October 30, 2024 19:24
@seadowg seadowg merged commit 0fcf9e3 into getodk:master Oct 31, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework FieldListUpdateTest
2 participants