test(angular): add timeout to avoid flaky value accessor test

[sean-perkins]

Pull request checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
• Build (npm run build) was run locally and any changes were pushed
• Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

Occasionally the Angular E2E test suite will fail when validating if setting a value changes the form status of the control.

Upon debugging this failing test, it appears to be the result of ion-select. After assigning the value, the control continues to have the ion-invalid class, which means that the value accessor never received the ionChange event.

Theory is that the value is set by Cypress before the component fully loads. By removing the safe guard, the event is still able to fire prior to the componentDidLoad resolving.

Issue URL: N/A

What is the new behavior?

• Adds TODO statements for code that will be removed if the ionChange RFC is approved
• Adds timeout to the e2e test to reduce the flakiness of the value accessor test

Does this introduce a breaking change?

• Yes
• No

Other information

[liamdebeasi]

Why was the this.didInit added in the first place? I am worried this may accidentally regress some other behavior.

[averyjohnston]

Would it be possible to have the test wait for the component to fully load instead?

[sean-perkins]

Inconclusive based on the original PR not having a description or referencing an issue.

I have two theories:

1. Based on the changed code in the original PR, it looks to be focused around floating labels/placeholders and ion-select. Originally the .emitStyle() was also included within this check (but later refactored out: 48a2763#diff-62cbe1fafe96a79e15c9c13dbafef8698860cfb31976445de7e1773079061eb6). it is possible there was a visual issue with the parent ion-item getting the correct classes applied (fix(angular): item styling when control has value #24932 may have resolved this)
2. There was a similar architecture followed for ion-segment, which references this issue: bug: ion-segment (ionChange) trigger is not consistent on start #19204. If the behavior described in that issue holds similar to ion-select, then the API changes to ionChange in v7 would eliminate that problem from existing.

Cypress should be able to automatically determine when the page has loaded to run the test suite. Outside of an arbitrary timeout, I don't know how to best wait for that component to be loaded here.

If we are uncomfortable with changing this behavior, it definitely can be removed in v7 with the RFC we are working on right now. In the interim, we could add a timeout + tech debt task to remove it in v7. Alternatively, the E2E test is only really checking 3 of our components for validity behavior. We could temporarily disable the ion-select.

[liamdebeasi]

Do you think the issue that the didInit tries to avoid will be resolved with our proposed ionChange revisions?

[sean-perkins]

Yes, I think the checks were to work around the value accessors setting the value before the web component defineCustomElement finishes.

With our proposed changes in the RFC, setting the value from the value accessor should not emit ionChange.

[liamdebeasi]

Can we keep the didInit patch for now and work around it in our test somehow? Then we can add a TODO to remove the didInit when the ionChange updates are merged (we can probably track these changes in the main ionChange ticket)

[liamdebeasi]

Thanks for taking care of this. Don't forget to change the commit subject when merging so this doesn't get logged as a bug.

[sean-perkins]

Thanks for the reminder; updated title + description, will wait for Amanda's review before merge 👍