Open yjaaidi opened 3 years ago
Thank you for taking time to provide this detailed and well thorough feedback! We really appreciate it, sorry that it took so long to respond.
This is something others have requested as well. We will probably add convenience methods like getHarness
, getAllHarness
, etc. I do want to take some time to think about what the API should look like, because there are a few options you can specify with the current API that I'd like to work in there somehow
The reason for this decision was to discourage people from saving harness instances and instead nudge them to call the getter when they want to get it. Saving it can be problematic if the user triggers some code that causes the underlying element it corresponds to to be removed from the DOM. It's a little more cumbersome, but my hope is that its worth it because it helps avoid some of these issues with staleness.
Do you have any ideas about how something like this would work from an implementation perspective? It seems a little odd to me because the return type for getMessageElement
would have to be a Promise
so you can do await getMessageElement()
, but it would also have to have a .text()
method.
I believe this has been added now https://github.com/angular/components/pull/20714 π
I'm reluctant to add a whole parallel set of sync methods since it doubles our API surface and it wouldn't be compatible with all environments. It seems like maybe this request is related to Cypress support, which I'm willing to spend a little time investigating though. (see 8)
I don't have anything against making that change, it's obviously a breaking change though, so it would have to go through our deprecation process and have a schematic to update people's code.
Our thinking for adding it to the CDK was that maintaining a set of harnesses is most useful for authors of component libraries. However, if we see that its popular among end-application developers to create harnesses for their components I'd be open to considering this.
I must admit I'm not familiar with Cypress, though I understand its very popular testing option. Unfortunately our team doesn't have the resources at this point to add official support for it, though I do hope its something the community will build on top. If you think the design of the system makes it hard to have it work with Cypress, I think that's something we could spend a little time working on and try to unblock others who may want to work on it.
Hi @mmalerba! There is no rush π Thank you for the detailed response.
Cool! Meanwhile, we can try some new interfaces by wrapping this with a third party library and see what works better.
Interesting π€. I am not convinced that it discourages people from saving the instances. What matters is the examples and common patterns. That's what people will stick to. I am more in favor of making simple APIs first and observe π
We would have to extend Promise
. I made an example here: https://stackblitz.com/edit/test-element-promise
Lovely! πππ
I just submitted a PR to add the missing TestElement
methods to docs: https://github.com/angular/components/pull/21103
Forget about this one. I'm investigating Cypress and I think I figured out a way to handle it with async functions. I'll keep you in touch π
It's clearly not the most important thing. If everyone is ok, we can keep it like this. Otherwise, it's cheaper to change it now than later. I am thinking about things like renaming async()
to waitForAsync()
as it invalidates lots of online (or sometimes printed) resources π
π
π‘ As mentioned in 5, I just had an idea yesterday that might work... I'll be back here with more info soon!
Nice to git-meet you @mmalerba by the way!
@mmalerba I'll be sharing something about Cypress soon π. I just have an issue with instanceof
calls like this one that prevents me from using Cypress commands https://docs.cypress.io/api/cypress-api/custom-commands.html#Syntax: https://github.com/angular/components/blob/43997571da4d599815d8bc4bb366f398f8652db7/src/cdk/testing/harness-environment.ts#L220
Cypress generates two different bundles for tests & commands that both include @angular/cdk/testing
so they don't share the same references to the HarnessPredicate
class for example and it breaks the instanceof
call.
Could we replace the instanceof
calls with something more duck-typy
like 'harnessType' in query
?
Yeah changing the instanceof
sounds reasonable to me. Just be sure to include a comment explaining why we're avoiding it.
That's a pretty cool TestElement
prototype. I'll file an FR to consider adding it. Will need to discuss with the team before deciding if we want to do it, but it does seem like it could help a lot with readability of test code.
Yeah changing the instanceof sounds reasonable to me. Just be sure to include a comment explaining why we're avoiding it.
Cool! I will. Thanks for your feedback.
That's a pretty cool TestElement prototype. I'll file an FR to consider adding it. Will need to discuss with the team before deciding if we want to do it, but it does seem like it could help a lot with readability of test code.
Happy to help π
We (jscutlery) just came up with this library @jscutlery/cypress-harness to support harnesses on Cypress. This solves items 5 & 8.
The only remaining issue is item 3 which turned into https://github.com/angular/components/issues/21183 as 1 & 2 can be solved with adapters & helpers.
Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.
Find more details about Angular's feature request process in our documentation.
Thank you for submitting your feature request! Looks like during the polling process it didn't collect a sufficient number of votes to move to the next stage.
We want to keep Angular rich and ergonomic and at the same time be mindful about its scope and learning journey. If you think your request could live outside Angular's scope, we'd encourage you to collaborate with the community on publishing it as an open source package.
You can find more details about the feature request process in our documentation.
As far as I can tell the @jscutlery/cypress-harness option doesn't have support for Angular beyond v13 anymore. It also appears to use the UnitTestElement
class meant for TestBed
rather than it's own implementation for Cypress (as the documentation in CDK suggests to do here).
There definitely would be a tonne of value in supporting Cypress as it's a major testing solution that a lot of people use (including my company). I had a go at writing my own but it's a little tricky since Cypress is largely built on Chainable commands and the TestElement
interface
expects Promise
s to be returned everywhere, which Cypress generally tells everyone to avoid doing.
This is an open discussion aiming to regroup Component Harness feedback and improvement ideas.
Most of the items below are focused on simplifying the API. The current APIs are a bit too complex; this can have a negative impact on
TestHarness
adoption.For test authors
1. Easier access to harness
HarnessLoader
is a nice abstraction but it can make harness instantiation cumbersome.Actual approach
It would be nice to have faster harness access like:
Suggestion 1.A
or even global functions like
getHarness
andgetProtractorHarness
functions could make the tests even more readable:Suggestion 1.B
For harness authors
2.
LocatorFactory
abstractionThe
LocatorFactory
approach (e.g.locatorFor()
method returns a function that takes no parameters) can be confusing and cumbersome.Actual approach
Suggestion 2.A
Simple accessor methods like
get()
orgetOptional()
seem easier to use and more intuitive.We can let developers factorize the way they want:
3.
async / await
vs chainingI am personally not a big fan of chaining (a.k.a. builder pattern) but in cases like this one where we end up with lots of
await
s, this can simplify the interface:Actual approach
Suggestion 3.A
4. Trigger any event
TestElement
should have atriggerEvent
function that allows harness authors to trigger any event.Suggestion 4.A
Common
5. Provide synchronous functions
Some environments can query the DOM synchronously (e.g.
TestbedHarnessEnvironment
) or through some under the hood chaining (e.g. Cypress) (Cf. https://docs.cypress.io/guides/core-concepts/introduction-to-cypress.html#Chains-of-Commands). Harness authors might want to focus on these environments. In that case, they will want to use synchronous functions and keep tests and harnesses easier to read & write.Current approach
Suggestion 5.A
Providing synchronous alternatives to accessors.
6.
TestbedHarnessEnvironment
vs.TestBedHarnessEnvironment
TestbedHarnessEnvironment
could be renamed toTestBedHarnessEnvironment
to stay consistent withTestBed
π7. Merge
TestBed
andTestbedHarnessEnvironment
In some future, wouldn't it be nice to merge
TestbedHarnessEnvironment
withTestBed
which means moving test harness to the angular repo?8. Cypress support
An external library could provide a
CypressHarnessEnvironment
but as presented in the 5th item, Cypress is based on an abstract chain of commands.TestElement
doesn't seem to be the right abstraction for this use case especially for getters liketext()
,getProperty()
etc...This is the last item on the list but probably the most important one. One of the key features of harnesses is the test environment abstraction and harness reuse through environments (TestBed, Protractor etc...) but if I am using TestBed and Cypress and if I can't reuse my harnesses with Cypress then it somewhat defeats the purpose of harnesses.