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

added sort.spec test #18

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

added sort.spec test #18

wants to merge 1 commit into from

Conversation

AlexGalichenko
Copy link
Owner

No description provided.

@AlexGalichenko
Copy link
Owner Author

Code Review Overview

Firstly, thank you for forwarding this code patch. The review below will address the changes made to both ProductsPage.ts and sort.spec.ts and will provide feedback on several aspects including structure, efficiency, readability, and adherence to best practices.

File: po/pages/ProductsPage.ts

Enhancements and Modifications

  1. Accessibility (burgerMenu and dropdown Getters):

    • The addition of the semicolon at the end of burgerMenu getter enhances consistency. This ensures alignment with JavaScript standards of ending statements with a semicolon.
    • New getter dropdown has been properly introduced, it's a good practice to abstract element selectors to page object properties for reusability and maintainability.
  2. Method get_prices:

    • Great addition for encapsulating the process of fetching and parsing price text. This is a good use of page object methods to abstract functionalities.
    • priceElements variable is typed as any. TypeScript's advantage is nullified when using any. Recommendation is to use specific type annotations for better compile-time checks.
    • Error handling is missing. The code assumes prices are always available and correctly formatted. Consider adding checks and error handling mechanisms.

Potential Bugs and Recommendations

  • Consider handling the potential exception where prices could be missing, incorrectly formatted, or the locator could fail.
  • The naming of get_prices is not consistent with TypeScript's convention (which often uses camelCase).
  • Add explicit access modifiers (public, private) to methods and properties to encapsulate the class functionality better.

File: tests/sort.spec.ts

Enhancements and Modifications

  1. Improvement in Test Structure:

    • Migration from a placeholder test to a descriptively wrapped set of operations including login and checkout functionalities.
    • Structuring tests using describe and beforeEach hooks is a good practice, promoting setup code reusability and readability.
  2. Test Logic:

    • The test aims to verify the sorting functionality which is critical for functionality. It fetches sorted prices, selects a sort option, and asserts the result.
    • Usage of standard user from users for login ensures test roles and privileges are checked correctly.

Potential Bugs and Recommendations

  • There is a potential logical bug related to sorting before asserting. The sort() function modifies the array in place. This might lead to checking the array against itself post-modification without verifying the actual sort functionality via UI interaction.
  • The assertion uses a deep equality check which is suitable. However, ensure the UI fetches prices dynamically post sort selection to actually verify the change.

General Recommendations

  • Code Clarity and Readability:

    • Rename get_prices to getPrices to stick with naming convention guidelines in TypeScript.
    • Use explicit types and avoid any to leverage TypeScript's static typing capabilities.
  • Test Robustness:

    • Consider verifying the price extraction by adding error-handling scenarios and null checks in tests and page methods.
    • To ensure sort.spec.ts correctly verifies the sorting functionality, ensure independence from modifications done in memory—sort after a fresh fetch or use cloned arrays for checking before and after states.

Review Status: Changes Requested

While the restructuring and added functionalities are acknowledged and appreciated, the issues concerning consistent coding standards, potential bugs due to unhandled scenarios, and logical issues in the test script necessitate revisions for robust and reliable functionality. Ensure to handle these changes prior to approval to enhance maintainability and reliability.

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.

2 participants