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

EPMRPP-79419 || Fixes for automation attributes #3254

Merged
merged 3 commits into from
Sep 9, 2022

Conversation

tr1ble
Copy link
Contributor

@tr1ble tr1ble commented Sep 6, 2022

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2022

Codecov Report

Merging #3254 (ac35ea2) into develop (573b0a1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #3254   +/-   ##
========================================
  Coverage    61.38%   61.38%           
========================================
  Files           73       73           
  Lines          795      795           
  Branches       120      120           
========================================
  Hits           488      488           
  Misses         280      280           
  Partials        27       27           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

defaultWidth={false}
dataAutomationId="longNameField"
/>
<FieldErrorHint provideHint={false} dataAutomationId="longNameField">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you don't use constants like NAME_FIELD_KEY?
each filed have a name and usually we use a constant for it
Also what the benefit of pass dataAutomationId to FieldErrorHint component and not directly to the FieldText?
In some places you passed dataAutomationId to the FieldElement, some to the FieldErrorHint please use one general way across the whole app

Copy link
Contributor Author

@tr1ble tr1ble Sep 6, 2022

Choose a reason for hiding this comment

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

We need to pass this attribute to upper container. It can be both FieldErrorHint or FieldElement so we pass it depending on the situation. I had this question too and asked Vika. She said that the discussed it with Ilya.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Attribute should be set to the field container which not only contains the field itself, but also contains field description, error, title, etc. which is related to the field. Some tests will not only work with the field itself, but will also verify the name of the field, the hint under it or the validation error text. So, it will be simpler to find the field container and find all these elements inside of it, than set separate test attribute to each element of each field. It was discussed with Ilya

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tr1ble tr1ble requested a review from chivekrodis September 6, 2022 09:28
@chivekrodis chivekrodis merged commit 40a6719 into develop Sep 9, 2022
@chivekrodis chivekrodis deleted the EPMRPP-79419_Automation_attributes_fixes branch September 9, 2022 14:53
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.

4 participants