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

feat: convenience method expectAllExist() for checking if multiple elements are detected (#APD-483) #751

Merged
merged 9 commits into from
Jun 20, 2024

Conversation

JohannesDienst-askui
Copy link
Collaborator

@JohannesDienst-askui JohannesDienst-askui commented Jun 6, 2024

Adds the convenience method

    params: ExpectExistenceInputParameter[],
  ): Promise<ExpectExistenceReturnValue>

It takes an array of elements in the form of ExpectExistenceInputParameter:

export type ExpectExistenceInputParameter = {
type: ExpectExistenceAllowedElementClasses;
label: string;
matching?: 'similar' | 'exact' | 'regex';
relation?: {
  type: RelationsForConvenienceMethods;
  text?: string;
};
exists?: boolean;
};

and returns ExpectExistenceReturnValue with a property everythingExists and the the Input-Parameter with exists that shows if the element was found:

export type ExpectExistenceReturnValue = {
  everythingExists: boolean;
  elements: ExpectExistenceInputParameter[];
};

Comprehensive tests can be found in our QA-Repository: https://github.com/askui/askui-qa-system-test/pull/2

@JohannesDienst-askui JohannesDienst-askui marked this pull request as ready for review June 7, 2024 12:16
Copy link
Contributor

@adi-wan-askui adi-wan-askui left a comment

Choose a reason for hiding this comment

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

First of all some likes:

  • Test suites
  • In code documentation very thorough

But I think the api and code is really complex for what it tries to achieve... Not sure if I am to review the api here, too, though. But I think we should have a quick call in order not to go back and forth because I think I may be lacking context.

packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
@JohannesDienst-askui
Copy link
Collaborator Author

@adi-wan-askui Thanks for the help with improving the API and the Code ❤️ .

I refactored to use reduce() and to work for multiple elements.
Also the tests have been updated to reflect the new method signature.

I struggled to not use this.evaluateMatchingProperty(command, param.text) as FluentFiltersOrRelationsGetter as the method could return FluentFiltersOrRelations and there for exec() could return void. Any idea on how to get rid of it or is there no way to tell the type system it is safe in this case?

@adi-wan-askui adi-wan-askui force-pushed the APD-483_expect_convenience_method branch from 32593b9 to 3c117dd Compare June 19, 2024 07:52
Copy link
Contributor

@adi-wan-askui adi-wan-askui left a comment

Choose a reason for hiding this comment

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

Well done switching to reduce :)!

To your question: You can replace the typecasts by signature overloads. I provided an example to you inside a commit to this branch. You can replace the other typecast the same way.

Left some last ideas for improving the naming, usage of switch statements and documenting parameters.

packages/askui-nodejs/src/execution/index.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
packages/askui-nodejs/src/execution/ui-control-client.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@adi-wan-askui adi-wan-askui left a comment

Choose a reason for hiding this comment

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

Well done :)

@JohannesDienst-askui JohannesDienst-askui changed the title feat: convenience method expectExistence() for checking if multiple elements are detected (#APD-483) feat: convenience method expectAllExist() for checking if multiple elements are detected (#APD-483) Jun 20, 2024
@JohannesDienst-askui JohannesDienst-askui merged commit 7787b46 into main Jun 20, 2024
3 checks passed
@JohannesDienst-askui JohannesDienst-askui deleted the APD-483_expect_convenience_method branch June 20, 2024 12:43
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.

None yet

3 participants