-
Notifications
You must be signed in to change notification settings - Fork 34
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
Re-enable embroider testing #34, #33, #72 #93
base: master
Are you sure you want to change the base?
Re-enable embroider testing #34, #33, #72 #93
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
ah, we have to resolve this:
|
@GavinJoyce + @alexlafroscia + @achambers do ya'll have thoughts on leaving the test selectors in, and then providing a (fractal) page object in addon-test-support for enabling easier interaction with these components in consuming apps? so that way consumers don't all need to write their own test helpers. (or they could, and just ignore what is provided, like if they don't like fractal for whatever reason (I only mention fractal, because ember-cli-page-object does not work well with typescript, and is generally too dynamic)). Another option is go page-object agnostic, and "just use the js apis" (but then we don't have any integrated query selector nesting, which is mostly what the page object patterns are for anyway (I mean, secondary to providing a humanized API for component interaction, and abstracting away the DOM) |
I've removed a dependency, so any previous approval is no longer valid, and to continue as if it were is sketchy. lol
I'm all for providing useful test helpers to consuming apps. I've no experience with fractal page objects, but happy to use them if they provide consuming apps a nice test API |
442c6a9
to
ed928c6
Compare
<script> | ||
// testdouble support | ||
// testdouble doesn't natively support the browser as it assumes | ||
// that a `global` identifier will be available, but that is not the case for browsers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reported this here: testdouble/testdouble.js#438
and it seemed there was 0 interest in resolving... :-\
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also opened a new issue: testdouble/testdouble.js#471
to see what they feel like doing about it 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this better than just configuring ember-auto-import
to polyfill global
as it did before? Did that configuration need to change to support Embroider?
alright, next blocker: ember-cli/ember-cli-deprecation-workflow#133 |
My perspective on the test helpers:
|
510f063
to
1479b60
Compare
…what else could be needed
…le the code that broke under embroider
c0eb392
to
8f11cc1
Compare
Supersedes #72