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

feature: support IDOMElementDescriptors #2087

Merged

Conversation

bendemboski
Copy link
Contributor

Implement support for RFC #726

@bendemboski
Copy link
Contributor Author

This PR does change existing error messages, as can be seen from the tests, which isn't an explicit goal of this change. Here's the explanation.

If an assertion couldn't find its element (whether because of a non-matching selector or because it was passed null), previously the error messaging would be inconsistent between its usage of <not found> vs. <unknown>. The code that produced those messages was refactored and is now driven by the dom element descriptor description functionality, so it would require extra code to restore the inconsistent behavior and keep the error message the same as it was before this PR.

@bendemboski bendemboski marked this pull request as ready for review January 20, 2024 18:38
bendemboski added a commit to bendemboski/fractal-page-object that referenced this pull request Mar 13, 2024
PageObjects are IDOMElementDescriptors, as described in emberjs/rfcs#726

This includes temporarily patching `qunit-dom` to include mainmatter/qunit-dom#2087 so we can test the full integration.
Copy link
Contributor

@BobrImperator BobrImperator left a comment

Choose a reason for hiding this comment

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

qunit-dom attempts to stay lean and it also promotes a way of testing without using page-objects. I understand the need and how its useful for some use cases.

I'd like us to look for a way for dom-element-descriptors to depend on qunit-dom instead and not the other way .around.
It seems like qunit-dom at the very minimum would need to provide a way to extend the element lookup and descriptions if they're required. and the assert.dom method.

Then what users of dom-element-descriptors would need to do is swap out qunit-dom setup with dom-element-descriptors setup.

// tests/test-helper.js
import * as QUnit from 'qunit';
import { setup } from 'dom-element-descriptors';

//...
setup(QUnit.assert);
setApplication(Application.create(config.APP));
start();
//...

Some pseudo code for potential qunit-dom extension could be:

import { setup, DOMHandler } from 'qunit-dom'

// A class or anything that implements similar interface
class DOMElementDescriptorHandler implements DOMHandler {
  findElement(selector, rootElement)
  findElements(selector, rootElement)
  description(selector)
  resultMessage()
}

setup(QUnit.assert, [new DOMElementDescriptorHandler()])

Draft #2101

packages/qunit-dom/lib/__tests__/has-tagname.ts Outdated Show resolved Hide resolved
packages/qunit-dom/lib/descriptor.ts Show resolved Hide resolved
@BobrImperator
Copy link
Contributor

Drafted #2101 as per feedback, would this help dom-element-descriptors to create a "plugin" for qunit-dom?

Copy link
Contributor

@BobrImperator BobrImperator left a comment

Choose a reason for hiding this comment

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

Please rebase the PR.

@bendemboski bendemboski force-pushed the dom-element-descriptors branch 2 times, most recently from d9f77d2 to 587d4d9 Compare June 27, 2024 04:55
@bendemboski
Copy link
Contributor Author

@BobrImperator this is rebased and, pending your decision on the assertion messages change or any other feedback you might have, ready to merge!

Extend qunit.dom() to accept IDOMElementDescriptors. See emberjs/rfcs#726.
@BobrImperator BobrImperator merged commit 81ca68e into mainmatter:master Jun 27, 2024
7 checks passed
@BobrImperator
Copy link
Contributor

BobrImperator commented Jun 27, 2024

bendemboski added a commit to bendemboski/fractal-page-object that referenced this pull request Jun 30, 2024
Now that mainmatter/qunit-dom#2087 is released, test against the released `qunit-dom` rather than our patched version. Also dedupe some dependencies.
bendemboski added a commit to bendemboski/fractal-page-object that referenced this pull request Jun 30, 2024
Now that mainmatter/qunit-dom#2087 is released, test against the released `qunit-dom` rather than our patched version. Also dedupe some dependencies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants