search

san650 / ember-cli-page-object

This ember-cli addon eases the construction of page objects on your acceptance and integration tests
http://ember-cli-page-object.js.org/
MIT License
275 stars 90 forks source link

Better errors from visitable #640

Closed simonihmig closed 8 months ago

simonihmig commented 9 months ago

This adds the actual error message from Ember's router to the (too generic) message from visitable. This is especially important when your test needs to know what kind of error was triggered. For example some tests might want to handle TransitionAborted errors differently, see https://github.com/emberjs/ember-test-helpers/issues/332#issuecomment-414641697.

ro0gr commented 9 months ago

Hey @simonihmig 👋 Thanks!

I think I'm not a fan to add external error messages into the page object error messages at the first place. This may be a breaking change for some test suites if we merge it as is. And this can break test suites later, when someone upgrades the @ember/test-helpers. That was one of the reasons we've migrated to a page object's own error message before(https://github.com/san650/ember-cli-page-object/pull/606), as a part of the v2 breaking realease.

I think a good enough tradeoff could be to keep the original error in the Error.cause. I believe, in this case the change can be backward compatible, and if someone really needs it they can use it(on their own risk), and it potentially allows for a better control over the original error type etc.. WDYT?

ro0gr commented 8 months ago

This may be a breaking change for some test suites if we merge it as is.

Ok I think I overrated the breaking nature of this change a bit. But I still feel like the cause is a proper place to store this kind of info.

simonihmig commented 8 months ago

Ok I think I overrated the breaking nature of this change a bit.

No worries! Normally I wouldn't think of the error message as something covered by the public API contract to be always the same. But actually, the changes in ecpo v2 were of a breaking nature to us because of this, as we had some checks like e.message === 'TransitionAborted' in place (for that very specific use case only) that broke with the upgrade.

I agree that we should make use of .cause, I forgot that this existed, so thanks for the suggestion, it totally makes sense!

However, I am still wondering if we should also update the message itself? You can only make use of .cause when actually running the test and putting a breakpoint somewhere to inspect it, right? But giving a better message in e.g. CI logs would be valuable for DX, to be able to immediately understand why the tests are failing. Could be an error thrown in a route hook, could be an error response in the (mocked) API, could be that transition error thing or whatnot...

But I'll leave that up to you to decide. My primary use case of being able to detect TransitionAborted cases is perfectly served with the .cause property! Will update this PR once this discussion has been settled! :)

ro0gr commented 8 months ago

But giving a better message in e.g. CI logs would be valuable for DX

I think you are right. Let's do it!

As a side note, I think it does also make sense to express the cause via jsdoc, so a future user is aware of this capability.

simonihmig commented 8 months ago

I just pushed this commit, which tries to implement the suggestion. However, there is a bit still left for discussion, because I realized that the error rethrown from the visitable helper is actually not the one received by the user, as it is catched and rethrown again by the "better-errors" functionality. And that already has opinions about cause here, which I didn't want to break by passing the original error. What I did instead was to just pass the message of e.cause if it exists, but keep the rest in place. Does that make sense?

ro0gr commented 8 months ago

Oh.. yes, what you say makes sense. I didn't expect the existing cause stuff would lead to an issue. I'll check if I can fix that and pr the fix against your branch, or merge your pr as is.

ro0gr commented 8 months ago

@simonihmig please take a look https://github.com/simonihmig/ember-cli-page-object/pull/1

simonihmig commented 8 months ago

Thanks @ro0gr, merged it, so this PR should be good to go now I hope!

ro0gr commented 8 months ago

Awesome. I've just left one minor question. Going to publish this tonight.

simonihmig commented 8 months ago

I've just left one minor question. Going to publish this tonight.

What is the question? 😄 This one? https://github.com/san650/ember-cli-page-object/pull/640#discussion_r1523018303

ro0gr commented 8 months ago

Hey, I've just released this https://github.com/san650/ember-cli-page-object/releases/tag/v2.3.0