Fix/navigation/top app bar UI tests

[agonzalez-r]

Fix UI tests for M1: TopAppBar

Description

This PR closes issue #90. It completes all features of the TopAppBar by adding an edit button!
As for the back button, this feature is conditional, and can be set up by passing as argument editButton = True to the TopAppBar() composable, along with the a function of the desired navigation action when this button is clicked (passed as the onEditButtonClick argument).

I must remark that no further tests were added, the tests for pre-existing features of the top bar were already very extensive and complete. Navigation should not be tested in TopAppBarTest.

Changes

Files

Added

none

Modified

• TopAppBar.kt: created arguments editButton (boolean, to indicate if this button should exist for the respective screen that calls the function) and onEditButtonClick (nullable lambda function, where the navigation action upon click should be specified)

Removed

none

Testing

• TopAppBarTest.kt: added tests for editButton feature by ensuring its presence when argument respective argument is true, that upon click the onEditButtonClick function is called, that exception is thrown if no such function is provided in this case, and other edge cases...

Screenshots

[francelu]

@agonzalez-r
Please assign yourself :) PO

[francelu]

Summary

You did a great job implementing the edit button! However, the tests need a few changes to improve quality, primarily by setting the on-click actions to null instead of {}. More details are in the code comments.

Important

Code Quality

• Improve navigation by using null for default on-click actions.

Functionality

• Ensure buttons are displayed only when actions are assigned.

Testing

• Update tests to check for button visibility based on action assignments.

Documentation

• Ensure any changes in parameters are reflected in the documentation.

Suggestions

• Review and implement the proposed changes in the code comments.

Steps before PR approved

1. Change the default values for onBackButtonClick and onEditButtonClick to null.
2. Update tests to verify that buttons do not display when actions are null.

[francelu]

I realize there are quite a few changes in the tests, mainly because I didn't test thoroughly while implementing navigation. Sorry for the extra work!

[agonzalez-r]

I realize there are quite a few changes in the tests, mainly because I didn't test thoroughly while implementing navigation. Sorry for the extra work!

no worries, they are minor changes but they do make the code a lot better

[taghizadlaura]

Summary

Unless what France said and my comment, it LGTM.
The TopAppBarTest class contains unit tests for the TopAppBar component, ensuring that it behaves as expected regarding its title, button visibility, and click functionality.

Important

Code Quality

• Readability: The code is well-structured with clear test names that indicate what each test is verifying. However, the names could be shortened, and fit more the other tests for improved readability.
• Consistency: PTAL, The test names do not match the naming conventions used in other classes, such as the Alert class (e.g., createInvalidAlertNoProduct). Establishing a consistent naming pattern across all test classes would improve maintainability and clarity.

Functionality

If you make the changes France requested :

• Correctness: LGTM, Each test checks specific features of the TopAppBar, ensuring that titles and buttons appear correctly based on the provided parameters.
• Edge Cases: LGTM, Tests handle scenarios where buttons are not displayed or when click handlers are null, asserting that appropriate exceptions are thrown or behaviors occur.

Performance

• Nothing to add.

Security

• Nothing to add.

Testing

• If you make the changes France requested, it LGTM.

Documentation

• LGTM, The test methods are self-explanatory (additional comments could enhance clarity about the purpose of specific tests or edge cases but it's not relevant).

Suggestions

• Make the changes France requested
• Match the naming conventions used in other classes for test names

Steps before PR approved

1. Change what France requested
2. Try to match the naming convention used in other classes for test names

[francelu]

LGTM. Well done ! :))

[francelu]

LGTM. I think this is from a merge with main into the branch.

[francelu]

LGTM. Seems like a merge with main into the branch too.

[taghizadlaura]

now it LGTM :)